Skip to content

Conversation

@arzonus
Copy link
Contributor

@arzonus arzonus commented Jan 19, 2026

What changed?

  • shard-distributor-canary got an option to run multiple executors for fixed and ephemeral namespaces
  • num-executors (for both namespaces), num-ephemeral-executors, num-fixed-executors flags were added to configure how many executors should be run
  • config folder was added

Why?

  • For testing purposes, It is easier to run multiple executors in one binary to simulate multiple running instances, so there is no need to change configuration files to run multiple instances of canary.
  • config was added to make advanced configuration for canary running internally without using flags.

How did you test it?

  • Unit test
  • Local run
  • Run on dev cluster
  • Run make sharddistributor-canary && ./sharddistributor-canary start --num-executors=10

@arzonus arzonus changed the title feat(shard-distributor-canary): allow to run multiple executors feat(shard-distributor-canary): enable running multiple executors Jan 19, 2026
@arzonus arzonus force-pushed the add-multiple-executors-to-canary branch 2 times, most recently from e3da20a to 796e5fc Compare January 22, 2026 08:39
Comment on lines +71 to +73
if numExecutors <= 0 {
numExecutors = 1
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just curious - how this can be if that's an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more for safe merging into internal code and to not have any blockers for merging into. I'm planning to make changes internally and remove these later :)


func opts(names NamespacesNames) fx.Option {
return fx.Options(
fx.Supply(names.Config),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit unusual - I wonder why we use fx.Supply instead of providing config? (and reader initially populating it)

Copy link
Contributor Author

@arzonus arzonus Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to bring a load of configuration to this PR, but to have some starting point for config. Later, we can add a PR adding a config to main.go. But I also don't want to have it as a failure point for internal code due to a change of app initialization, so I want to make it as much as smothier way.

Right now, there is no real reason for us to use config locally; however internally we need to be able to modify these values, because we don't use flags internally. So I introduced config here and I didn't want to make it a required parameter for the canary.Module to not break the app initialization internally, but at the same time, I want to have it as a required parameter for Executors and ShardCreator later.

So for this, I decided to supply config, but internally I can use a decorator to override values and load configuration by using internal configuration functions.

@arzonus arzonus force-pushed the add-multiple-executors-to-canary branch from 986f967 to e26eacc Compare January 29, 2026 12:04
@arzonus arzonus changed the title feat(shard-distributor-canary): enable running multiple executors feat(shard-distributor-canary): add support of multiple executors Jan 29, 2026
@arzonus
Copy link
Contributor Author

arzonus commented Jan 29, 2026

gitar apply your changes

@gitar-bot
Copy link

gitar-bot bot commented Jan 29, 2026

gitar apply your changes

✅ Applied the typo fix: Changed numEphemeralxecutors to numEphemeralExecutors in cmd/sharddistributor-canary/main.go (lines 49 and 51).

@arzonus arzonus force-pushed the add-multiple-executors-to-canary branch from bff0ab7 to f381d24 Compare January 29, 2026 12:43
@gitar-bot
Copy link

gitar-bot bot commented Jan 29, 2026

Code Review ✅ Approved 2 resolved / 2 findings

The flag precedence issue has been properly fixed using c.IsSet() to implement true override behavior as documented. Implementation is clean with proper tests and defensive defaults.

✅ 2 resolved
Bug: Flag precedence logic contradicts documentation

📄 cmd/sharddistributor-canary/main.go:47-48 📄 cmd/sharddistributor-canary/main.go:185-186
The --num-executors flag documentation says it "Overrides num-fixed-executors and num-ephemeral-executors flags", but the implementation uses max() which takes the larger value rather than always using num-executors as an override.

Current behavior:

  • --num-executors=10 --num-fixed-executors=5 → 10 (as documented)
  • --num-executors=5 --num-fixed-executors=10 → 10 (contradicts documentation - should be 5)

Options to fix:

  1. If num-executors should truly override: Change the logic to use num-executors when explicitly set:
numFixedExecutors := c.Int("num-fixed-executors")
if c.IsSet("num-executors") {
    numFixedExecutors = c.Int("num-executors")
}
  1. If max is intended: Update the flag documentation to say "Sets a minimum" or "Takes the maximum of" instead of "Overrides".
Bug: Typo in variable name: `numEphemeralxecutors`

📄 cmd/sharddistributor-canary/main.go:48
The variable numEphemeralxecutors is missing the letter 'E' - it should be numEphemeralExecutors for consistency with numFixedExecutors.

While this doesn't cause a runtime bug (the variable is used consistently with the typo), it makes the code harder to read and maintain.

Suggested fix:

numEphemeralExecutors := max(c.Int("num-ephemeral-executors"), numExecutors)

And update the function call accordingly.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants