Repository navigation
feat(consumer): add --all-streams flag to cluster balance - #1753
manish-9245 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Please update your commit and PR message with something less verbose and more inline with commit commit message standards no one need to know what tests ran in a commit message that’s what CI is for better you just wrote these messages yourself and not meat proxy Thanks. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ed2e3f41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| balanced, err := balancer.BalanceConsumers(consumers) | ||
| var streams []*jsm.Stream | ||
| _, _, err = c.mgr.EachStream(nil, func(s *jsm.Stream) { |
There was a problem hiding this comment.
When a non-internal stream is missing or offline during --all-streams, EachStream reports it through its first two return values instead of invoking this callback, but both values are discarded here. The command consequently skips that stream and can still print a successful summary and exit zero (even reporting zero streams if every stream is unavailable), so callers cannot tell that the requested all-stream operation was incomplete; inspect these results and return an error for affected user streams.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed. missing/offline from EachStream are now collected into the combined error instead of discarded, so --all-streams fails instead of printing success on incomplete enumeration.
e7ed2e3 to
3d0e00a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0e00a867
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for _, stream := range streams { | ||
| balanced, err := c.balanceStreamConsumers(stream, b, opts, true) |
There was a problem hiding this comment.
When the skew spans streams (for example, three streams each have one matching consumer and all three leaders are on the same server), this loop calls BalanceConsumers with a one-consumer slice for each stream. The balancer judges each slice's distribution independently, so each call can report balanced and move nothing even though the selected consumers are globally skewed; gather all matching consumers from streams and invoke the balancer once for --all-streams.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed. --all-streams now gathers matching consumers from all streams and calls BalanceConsumers once, so cross-stream skew is balanced. Single-stream path unchanged.
3d0e00a to
974a21a
Compare
|
Done - shortened the commit and PR text. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 974a21a98c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if len(missing) > 0 { | ||
| errs = append(errs, fmt.Sprintf("inaccessible streams: %s", strings.Join(missing, ", "))) |
There was a problem hiding this comment.
The newly added error collection handles inaccessible user streams, but when the parent --all flag is unset the callback excludes internal streams while missing (and the offline map below) are appended without applying the same filter. If an internal KV/object/system stream is unavailable, --all-streams therefore exits nonzero even though that stream was outside the requested scope; discard names for which jsm.IsInternalStream is true unless c.showAll is set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed. missing/offline names are now filtered with jsm.IsInternalStream when -a/--all is unset, matching the EachStream callback, so internal KV/OBJ/MQTT streams no longer fail --all-streams outside scope.
974a21a to
9b4b40b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b4b40b79a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| total := 0 | ||
| var allConsumers []*jsm.Consumer | ||
| for _, stream := range streams { | ||
| consumers, err := stream.QueryConsumers(opts...) |
There was a problem hiding this comment.
When an accessible stream has a consumer whose information cannot be loaded (for example, a per-consumer info timeout), QueryConsumers returns only the consumers it loaded and does not expose the missing/offline collections available from EachConsumer; its err only represents the overall query. This loop can therefore build an incomplete global set, let BalanceConsumers rebalance that subset, and exit successfully even though --all-streams omitted a requested consumer. Validate and propagate these per-consumer enumeration failures before balancing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed. --all-streams now validates per-consumer enumeration via EachConsumer before adding to the global set, and any missing/offline consumers join the combined error instead of silently balancing a subset.
9b4b40b to
1b88ec9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b88ec9da1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fmt.Printf("Balanced %d consumers on %d streams\n", total, len(streams)) | ||
|
|
||
| if len(errs) > 0 { | ||
| return fmt.Errorf("failed to balance consumers on %d streams: %s", len(errs), strings.Join(errs, "; ")) |
There was a problem hiding this comment.
When one stream contributes multiple diagnostics—for example, two entries in offlineC—the loop appends several elements to errs, while one inaccessible streams element can represent several streams and a balancer error represents no individual stream. Consequently len(errs) can over- or under-report the number of streams in the final failure message, making the all-streams result misleading; track distinct affected stream names or avoid claiming a stream count.
Useful? React with 👍 / 👎.
Fixes #1748
Adds --all-streams to nats consumer cluster balance to balance consumers on all streams at once.
Uses --all-streams since --all is already taken by the parent consumer command.