Skip to content

[AMORO-3921] Add AmsAssignService and ZkBucketAssignStore to implement balanced bucket allocation in master-slave mode#3922

Open
wardlican wants to merge 13 commits intoapache:masterfrom
wardlican:amoro#3921
Open

[AMORO-3921] Add AmsAssignService and ZkBucketAssignStore to implement balanced bucket allocation in master-slave mode#3922
wardlican wants to merge 13 commits intoapache:masterfrom
wardlican:amoro#3921

Conversation

@wardlican
Copy link
Contributor

Why are the changes needed?

This implements the logic for dynamically and evenly distributing buckets based on the number of AMS nodes.

Close #3921 .

Brief change log

AmsAssignService Functionality

  1. Periodic Allocation: Performs bucket ID allocation every 30 seconds (configurable)
  2. Node Change Detection: Detects new and offline nodes
  3. Incremental Allocation: Minimizes migration, preserving existing node buckets as much as possible
  4. Load Balancing: Distributes buckets evenly across all live nodes
  5. Offline Node Handling: Detects offline nodes based on a timeout mechanism and reallocates their buckets
  6. Master Node Only: Only the leader node executes the allocation logic.

ZkBucketAssignStore Features

  1. Storage Implementation: Stores bucket assignment information based on ZooKeeper.
  2. Data Storage:
    • List of bucket IDs for each node (JSON format)
    • Last update timestamp for each node
  3. CRUD Operations:
    • Save/Update Node Assignments
    • Query Node Assignments
    • Delete Node Assignments
    • Query All Node Assignments
  4. Access Control: Only the leader node can save/delete assignment information.
  5. Path Structure: /{cluster}/amoro/ams/bucket-assignments/{nodeKey}/assignments and /{cluster}/amoro/ams/bucket-assignments/{nodeKey}/last-update-time

two classes that work together to implement dynamic bucket ID allocation and storage in master-slave mode.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 22.45%. Comparing base (e02295b) to head (9fb8741).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/apache/amoro/properties/AmsHAProperties.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3922   +/-   ##
=========================================
  Coverage     22.44%   22.45%           
- Complexity     2552     2555    +3     
=========================================
  Files           458      458           
  Lines         42022    42045   +23     
  Branches       5915     5917    +2     
=========================================
+ Hits           9433     9440    +7     
- Misses        31777    31793   +16     
  Partials        812      812           
Flag Coverage Δ
trino 22.45% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 18, 2025
@wardlican
Copy link
Contributor Author

Please conduct a code review of the implementation here.

@github-actions github-actions bot removed the stale label Dec 20, 2025
@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 19, 2026
@github-actions
Copy link

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Jan 26, 2026
@czy006 czy006 reopened this Feb 26, 2026
@github-actions github-actions bot removed the stale label Feb 27, 2026
@github-actions github-actions bot added the type:docs Improvements or additions to documentation label Mar 6, 2026
@czy006 czy006 requested review from czy006 and xxubai March 6, 2026 07:13
Copy link
Contributor

@xxubai xxubai left a comment

Choose a reason for hiding this comment

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

Thanks for keeping pushing this feature

@xxubai xxubai changed the title add AmsAssignService and ZkBucketAssignStore to implement balanced bucket allocation in master-slave mode [AMORO-3921] Add AmsAssignService and ZkBucketAssignStore to implement balanced bucket allocation in master-slave mode Mar 12, 2026
* ZkBucketAssignStore.getNodeKey().
*/
private String getNodeKey(AmsServerInfo nodeInfo) {
return nodeInfo.getHost() + ":" + nodeInfo.getThriftBindPort();
Copy link
Contributor

Choose a reason for hiding this comment

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

AmsServerInfo.equals() compares host, thriftBindPort, and restBindPort. But ZkBucketAssignStore.parseNodeKey() (line 225-233) and AmsAssignService.getNodeKey() only store host:thriftBindPort, losing restBindPort.
Will the AmsServerInfo collections return the incorrect result? like removeAll(nodes), containsKey(nodeKey)

Copy link
Contributor Author

@wardlican wardlican Mar 13, 2026

Choose a reason for hiding this comment

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

There won't be this problem. This is mainly used to distinguish different AMS nodes. The IP address and Thriftport are already sufficient to uniquely identify different nodes, so there's no need to add a restport.

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

Labels

module:ams-server Ams server module module:common type:docs Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Subtask]: add AmsAssignService to implement balanced bucket allocation in master-slave mode.

4 participants