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

[#410] feat: support the hot reload of coordinator's configuration #572

Merged
merged 21 commits into from
Feb 10, 2023

Conversation

jerqi
Copy link
Contributor

@jerqi jerqi commented Feb 9, 2023

What changes were proposed in this pull request?

I refer to Hadoop implement. I design a hot reload process. We support nodeMax at first.

Why are the changes needed?

It's more convenient.

Does this PR introduce any user-facing change?

add rss.reconfigure.interval.sec.

How was this patch tested?

UT

@jerqi jerqi changed the title [#410] fea: Support the hot reload of coordinator's configuration [#410] feat: Support the hot reload of coordinator's configuration Feb 9, 2023
@jerqi jerqi changed the title [#410] feat: Support the hot reload of coordinator's configuration [#410] feat: support the hot reload of coordinator's configuration Feb 9, 2023
@advancedxy
Copy link
Contributor

Is this ready for review?

@jerqi
Copy link
Contributor Author

jerqi commented Feb 9, 2023

Is this ready for review?

OK.

@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023

Codecov Report

Merging #572 (4e861f1) into master (f461460) will decrease coverage by 0.01%.
The diff coverage is 12.67%.

@@             Coverage Diff              @@
##             master     #572      +/-   ##
============================================
- Coverage     60.87%   60.87%   -0.01%     
- Complexity     1796     1799       +3     
============================================
  Files           213      214       +1     
  Lines         12375    12398      +23     
  Branches       1049     1051       +2     
============================================
+ Hits           7533     7547      +14     
- Misses         4435     4443       +8     
- Partials        407      408       +1     
Impacted Files Coverage Δ
...ache/uniffle/common/config/ReconfigurableBase.java 0.00% <0.00%> (ø)
.../org/apache/uniffle/coordinator/AccessManager.java 71.73% <0.00%> (-22.55%) ⬇️
...ache/uniffle/coordinator/SimpleClusterManager.java 82.83% <0.00%> (-3.89%) ⬇️
...nator/access/checker/AccessClusterLoadChecker.java 82.05% <0.00%> (-14.92%) ⬇️
.../apache/uniffle/coordinator/CoordinatorServer.java 57.02% <23.07%> (-4.45%) ⬇️
.../org/apache/uniffle/common/config/RssBaseConf.java 94.04% <100.00%> (+0.22%) ⬆️
...ava/org/apache/uniffle/server/ShuffleTaskInfo.java 98.07% <0.00%> (-1.93%) ⬇️
...he/uniffle/server/buffer/ShuffleBufferManager.java 83.09% <0.00%> (-0.89%) ⬇️
...torage/handler/impl/ComposedClientReadHandler.java 0.00% <0.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

advancedxy
advancedxy previously approved these changes Feb 9, 2023
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

Generally lgtm, except two minor comments.

kaijchen
kaijchen previously approved these changes Feb 9, 2023
Copy link
Contributor

@kaijchen kaijchen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jerqi for the work. Some nits inlined.

…urableBase.java

Co-authored-by: Kaijie Chen <ckj@apache.org>
@jerqi jerqi dismissed stale reviews from kaijchen and advancedxy via 87c6329 February 10, 2023 02:00
jerqi and others added 2 commits February 10, 2023 10:00
…urableBase.java

Co-authored-by: Kaijie Chen <ckj@apache.org>
…urableBase.java

Co-authored-by: Kaijie Chen <ckj@apache.org>
Copy link
Contributor

@kaijchen kaijchen left a comment

Choose a reason for hiding this comment

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

LGTM

@jerqi jerqi merged commit ab17e77 into apache:master Feb 10, 2023
@jerqi
Copy link
Contributor Author

jerqi commented Feb 10, 2023

Thanks @advancedxy @kaijchen , merged.

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.

[FEATURE] Support the hot reload of coordinator's configuration rss.coordinator.shuffle.nodes.max
4 participants