-
Notifications
You must be signed in to change notification settings - Fork 36
scheduling: Prevent duplicate add operators by skipping split postFinish when target nodes mismatch #4078
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: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @hongyunyan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential issue in the scheduling mechanism where duplicate add operators could be created during a split operation if the provided target nodes do not align with the actual number of resulting spans. The changes ensure that the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request addresses a bug that could lead to a panic or race condition during a table span split operation when the number of target nodes mismatches the number of resulting spans. The fix is implemented by introducing a replicasInScheduling boolean flag returned by ReplaceReplicaSet. This flag indicates whether the new replicas have been placed in a scheduling state. The SplitDispatcherOperator now checks this flag before invoking the postFinish callback, effectively preventing duplicate scheduling operations. A warning log has also been added for cases where postFinish is skipped due to the mismatch. The changes are accompanied by a new unit test that specifically covers the fixed scenario, ensuring the bug is resolved and preventing future regressions. The implementation is clean, correct, and the added test provides good coverage for the fix.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #4077
What is changed and how it works?
This pull request addresses a potential issue in the scheduling mechanism where duplicate add operators could be created during a split operation if the provided target nodes do not align with the actual number of resulting spans. The changes ensure that the
postFinishcallback, which typically initiates further scheduling, is only invoked when the target nodes are correctly aligned. In cases of mismatch, the new spans are marked as absent, allowing the system's basic scheduler to take over and prevent redundant operations, thereby improving the robustness of the scheduling process.Highlights
SplitDispatcherOperator.PostFinishmethod now conditionally invokes itspostFinishcallback. This callback is only executed if the new replica sets are successfully placed into a scheduling state with aligned target nodes.spanController.ReplaceReplicaSetfunction has been updated to return an additional boolean value,replicasInScheduling, which indicates whether the newly created replica sets were successfully assigned to the specified target nodes.splitTargetNodesdoes not match the number of newly created spans,ReplaceReplicaSetwill now place these new spans into an "absent" state instead of a "scheduling" state. This allows the basic scheduler to handle them and prevents the creation of duplicate add operators.TestSplitOperator_PostFinishSkipsWhenTargetNodesMismatch, has been added to specifically verify the behavior wherepostFinishis skipped when target nodes do not align with the actual split results.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note