Skip to content

Conversation

@arzonus
Copy link
Contributor

@arzonus arzonus commented Jan 20, 2026

What changed?

  • Multiple instances of ShardCreator are enabled to run

Why?

  • ShardCreator is used for new ephemeral shard creation. This PR enables running multiple instances for load testing purposes.

How did you test it?

  • Run locally
  • Run on dev cluster

@arzonus arzonus marked this pull request as draft January 20, 2026 09:34
@arzonus arzonus force-pushed the add-shard-creator-config branch from 8d163d3 to afe283c Compare January 22, 2026 09:00
@arzonus arzonus marked this pull request as ready for review January 22, 2026 09:03
@arzonus arzonus changed the title feat(shard-distributor-canary): enable multiple shard creators feat(shard-distributor-canary): add support of multiple shard creators in sd-canary Jan 29, 2026
@arzonus arzonus force-pushed the add-shard-creator-config branch from afe283c to cd48a01 Compare January 29, 2026 12:45
@arzonus arzonus changed the title feat(shard-distributor-canary): add support of multiple shard creators in sd-canary feat(shard-distributor-canary): add support of multiple shard creators Jan 29, 2026
@gitar-bot
Copy link

gitar-bot bot commented Jan 29, 2026

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Well-structured feature addition for load testing with multiple shard creators and executors. One minor code quality issue with input parameter mutation that could be easily improved.

💡 Bug: Input parameter mutation in NewShardCreator

📄 service/sharddistributor/canary/processorephemeral/shardcreator.go:46-48

The NewShardCreator function mutates the input params.Config.Canary.ShardCreationInterval directly when the value is <= 0. While this works correctly in the current usage because the config is used immediately after mutation, it's a code smell to mutate input parameters. This could lead to subtle bugs if the caller expects the original config to remain unchanged, or if the same config is reused elsewhere.

Impact: Currently low risk since the mutation happens before the loop in ShardCreatorModule starts iterating, but future refactoring could expose this issue.

Suggested fix: Make a local copy of the interval value instead of mutating the config:

func NewShardCreator(params ShardCreatorParams, namespaces []string) *ShardCreator {
    interval := params.Config.Canary.ShardCreationInterval
    if interval <= 0 {
        interval = shardCreationInterval
    }

    return &ShardCreator{
        // ... other fields
        creationInterval: interval,
    }
}
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.

1 participant