feat: add SubscriptionUserDataProvider hook for BalanceStrategy#3506
Conversation
Add an optional SubscriptionUserDataProvider interface that a BalanceStrategy may implement. When present, sarama invokes SubscriptionUserData(topics) immediately before each JoinGroup, and the returned bytes replace the statically configured Consumer.Group.Member.UserData for that strategy's subscription metadata only. A nil result or non-nil error falls back to the static UserData (errors are logged, not fatal). This mirrors the Java ConsumerPartitionAssignor.subscriptionUserData hook and enables load-aware assignors that report fresh per-cycle observations (CPU, lag, latency, etc.) to the group leader on every rebalance cycle. Existing BalanceStrategy implementations are unaffected: the interface is optional and the behavior for strategies that do not implement it is unchanged. The interface is treated as V1; future revisions should be introduced as separate interfaces (e.g., SubscriptionUserDataProviderV2) to preserve backward compatibility. Includes a unit test of the per-strategy metadata builder and a worked example under examples/consumer_load_aware/ that wraps the built-in sticky strategy with a load-reporting SubscriptionUserDataProvider. Fixes IBM#3505 Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
| // separate interfaces (e.g., SubscriptionUserDataProviderV2) to preserve | ||
| // backward compatibility with existing implementations. | ||
| type SubscriptionUserDataProvider interface { | ||
| SubscriptionUserData(topics []string) ([]byte, error) |
There was a problem hiding this comment.
This receiver method is named as a noun. This is usually non-ideal, as the whole idiomatic use of Golang naming is to name the interface as a Doer, that has receiver method Do, or Handler that has receiver method Handle.
The receiver method is a verb, and the interface name is then the agent nominalization of that verb.
There was a problem hiding this comment.
Whilst I sort of agree, we do already have the AssignmentData noun in the BalanceStrategy interface, so there is some existing precedent for noun-named methods that return bytes and it does match Java's subscriptionUserData() so I think we're probably best to accept this as-is
- Rename SubscriptionUserDataProvider to SubscriptionUserDataBalanceStrategy and embed BalanceStrategy so implementations must also be a BalanceStrategy. - Drop the YAGNI paragraph about hypothetical V2. - Treat nil and empty returned slices identically: a nil error means use whatever was returned (including nil/empty) verbatim. Only a non-nil error falls back to the static UserData. Update doc comment accordingly. - Inline the single-line addProtocol helper in joinGroupRequest. - Restructure subscriptionMetadata around an early return rather than a mutable userData local; tighten the fallback log line. - Use slices.Clone in the test recorder and errors.New for the boom error. - Add a test case for the empty-slice-clears-static behaviour now that nil and empty are handled the same way. Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
8bf81de to
f7cc882
Compare
puellanivis
left a comment
There was a problem hiding this comment.
Code seems to look fine, though I’m unsure of the integration into the larger logic though.
dnwe
left a comment
There was a problem hiding this comment.
LGTM, couple of minor comments:
Per dnwe's review on IBM#3506: - Pass slices.Clone(topics) to the SubscriptionUserData hook so a user-provided provider cannot mutate the slice we later attach to the JoinGroup request. - Rewrite the fallback log line to follow the codebase's "consumergroup/%s: ..." prefix convention. Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
Per dnwe's review on IBM#3506: - Pass slices.Clone(topics) to the SubscriptionUserData hook so a user-provided provider cannot mutate the slice we later attach to the JoinGroup request. - Rewrite the fallback log line to follow the codebase's "consumergroup/%s: ..." prefix convention. Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
Add an optional SubscriptionUserDataProvider interface that a BalanceStrategy may implement. When present, sarama invokes SubscriptionUserData(topics) immediately before each JoinGroup, and the returned bytes replace the statically configured Consumer.Group.Member.UserData for that strategy's subscription metadata only. A nil result or non-nil error falls back to the static UserData (errors are logged, not fatal). This mirrors the Java ConsumerPartitionAssignor.subscriptionUserData hook and enables load-aware assignors that report fresh per-cycle observations (CPU, lag, latency, etc.) to the group leader on every rebalance cycle.
Existing BalanceStrategy implementations are unaffected: the interface is optional and the behavior for strategies that do not implement it is unchanged. The interface is treated as V1; future revisions should be introduced as separate interfaces (e.g., SubscriptionUserDataProviderV2) to preserve backward compatibility.
Includes a unit test of the per-strategy metadata builder and a worked example under examples/consumer_load_aware/ that wraps the built-in sticky strategy with a load-reporting SubscriptionUserDataProvider.
Fixes #3505