-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Enable configurable multi-lease acquisition per cycle in change feed processor #47606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution @stas-openai! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces an opt-in configuration (setMaxLeasesToAcquirePerCycle) to the Change Feed Processor (CFP) that allows acquiring multiple leases per balancing cycle, improving rebalance/convergence time during scale-out and rolling deployments. The default value of 0 preserves the legacy conservative behavior of acquiring at most one lease per cycle when multiple workers exist.
Key Changes:
- Added
maxLeasesToAcquirePerCyclefield and accessors toChangeFeedProcessorOptionswith validation - Updated
EqualPartitionsBalancingStrategyto honor the new option for expired/unowned lease acquisition while keeping the legacy 1-lease-per-cycle behavior for stealing - Wired the new option through both CFP implementations (EPK and PK/Incremental versions)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ChangeFeedProcessorOptions.java |
Adds the new maxLeasesToAcquirePerCycle field with getter/setter and validation |
IncrementalChangeFeedProcessorImpl.java |
Passes the new option to the EqualPartitionsBalancingStrategy constructor |
ChangeFeedProcessorImplBase.java |
Passes the new option to the EqualPartitionsBalancingStrategy constructor |
EqualPartitionsBalancingStrategy.java |
Implements the multi-lease acquisition logic while maintaining legacy behavior for stealing |
CHANGELOG.md |
Documents the new feature in the changelog |
PartitionLoadBalancerImplTests.java (pkversion) |
Adds test verifying all leases returned by strategy are attempted |
PartitionLoadBalancerImplTests.java (epkversion) |
Adds test verifying all leases returned by strategy are attempted |
EqualPartitionsBalancingStrategyTests.java |
Adds comprehensive unit tests for the new multi-lease acquisition behavior |
...java/com/azure/cosmos/implementation/changefeed/common/EqualPartitionsBalancingStrategy.java
Outdated
Show resolved
Hide resolved
...java/com/azure/cosmos/implementation/changefeed/common/EqualPartitionsBalancingStrategy.java
Show resolved
Hide resolved
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
|
@microsoft-github-policy-service agree company="OpenAI" |
|
Thank you @stas-openai for your contributions to improve the Change feed processor logic, we will review this PR in the upcoming week (once everyone is back from vacation) and will get back to you with any comments, thanks again, your contributions are deeply appreciated! |
| /** | ||
| * Maximum number of leases the instance will try to acquire in a single load balancing cycle. | ||
| * <p> | ||
| * A value of {@code 0} keeps the legacy behavior (which is intentionally conservative when multiple workers exist and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Short comment explaining when/why using a higher value could be useful - also what are the reasons to not use a super high value etc. Basically providing a bit of guidance on how to estimate a good value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind - saw that the comment for the public API has some guidance already.
| ### 4.77.0-beta.1 (Unreleased) | ||
|
|
||
| #### Features Added | ||
| * Added `ChangeFeedProcessorOptions#setMaxLeasesToAcquirePerCycle(int)` to allow faster acquisition of unused/expired leases during scale-out and rolling deployments (default `0` preserves legacy behavior). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - template should be followed (including link to the PR)
FabianMeiswinkel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - Thanks for the contribution!
| } | ||
| // For multiple acquisitions, shuffle and take a random subset. | ||
| Collections.shuffle(expiredLeases, random); | ||
| this.logger.info("Found {} unused or expired leases; previous lease count for instance owner {} is {}, count of leases to target is {} and maxScaleCount {} ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These log lines can include maxLeasesToAcquirePerCycle as well.
Description
This PR adds an opt-in configuration to Change Feed Processor (CFP) to allow acquiring more than one lease per lease-
acquire cycle, which can significantly improve rebalance/convergence time during scale-out and rolling deployments
(especially when hosts use ephemeral identities, e.g., Kubernetes rollouts).
Today, when multiple CFP workers exist, the default EqualPartitionsBalancingStrategy is intentionally conservative and
effectively attempts to take ownership of at most one lease per cycle. In large lease sets / high physical partition
counts, this can make rebalancing very gradual and can temporarily reduce processing throughput after deployments.
Changes included:
strategy.
No Swagger regeneration involved.
All SDK Contribution checklist:
General Guidelines and Best Practices
merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR,
see this page.
Testing Guidelines