Skip to content
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

[AMORO-3459][Improvement]: Obtain the master's service address based on service discovery, which requires writing the HTTP port to ZooKeeper #3462

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

czy006
Copy link
Contributor

@czy006 czy006 commented Mar 11, 2025

[AMORO-3459][Improvement]: Obtain the master's service address based on service discovery, which requires writing the HTTP port to ZooKeeper(#3459) ConradJam 2 minutes ago

Why are the changes needed?

Close #3459.

Brief change log

  • Add AMS Rest Port in AmsServerInfo
  • AMSROOT Path change artic to amoro

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? (no)
  • If yes, how is the feature documented? (not applicable)

…on service discovery, which requires writing the HTTP port to ZooKeeper(apache#3459)
@zhoujinsong
Copy link
Contributor

@czy006 I'm afraid that this change might break client compatibility, causing older versions of the client to fail to retrieve information correctly. We may need to consider the risks associated with the version upgrade.

@czy006
Copy link
Contributor Author

czy006 commented Mar 13, 2025

@czy006 I'm afraid that this change might break client compatibility, causing older versions of the client to fail to retrieve information correctly. We may need to consider the risks associated with the version upgrade.

This is indeed a destructive upgrade. We have changed the metadata and zk address. Could we explain this in the documentation? Or do we have to enforce compatibility? @zhoujinsong

@baiyangtx
Copy link
Contributor

@czy006 I'm afraid that this change might break client compatibility, causing older versions of the client to fail to retrieve information correctly. We may need to consider the risks associated with the version upgrade.

This is indeed a destructive upgrade. We have changed the metadata and zk address. Could we explain this in the documentation? Or do we have to enforce compatibility? @zhoujinsong

Considering that the data stored in zk is ephemeral, this upgrade only requires the optimizer, client, and ams to be upgraded together without involving data migration. Can it be included in version 0.8?

@zhoujinsong

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 27.92%. Comparing base (f07990a) to head (b228b23).

Files with missing lines Patch % Lines
...in/java/org/apache/amoro/client/AmsServerInfo.java 0.00% 7 Missing ⚠️
...apache/amoro/server/HighAvailabilityContainer.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3462      +/-   ##
============================================
- Coverage     27.92%   27.92%   -0.01%     
- Complexity     3663     3666       +3     
============================================
  Files           603      603              
  Lines         49348    49354       +6     
  Branches       6367     6368       +1     
============================================
  Hits          13780    13780              
- Misses        34598    34607       +9     
+ Partials        970      967       -3     
Flag Coverage Δ
core 27.92% <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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants