Skip to content

[CELEBORN-1619][CIP-11] Integrate tags manager with config service#2844

Closed
s0nskar wants to merge 6 commits intoapache:mainfrom
s0nskar:tags_via_config
Closed

[CELEBORN-1619][CIP-11] Integrate tags manager with config service#2844
s0nskar wants to merge 6 commits intoapache:mainfrom
s0nskar:tags_via_config

Conversation

@s0nskar
Copy link
Contributor

@s0nskar s0nskar commented Oct 23, 2024

What changes were proposed in this pull request?

Integrating TagsManager with configService. Users can now update the workers tags using ConfigService.

Why are the changes needed?

https://cwiki.apache.org/confluence/display/CELEBORN/CIP-11+Supporting+Tags+in+Celeborn

Does this PR introduce any user-facing change?

NA

How was this patch tested?

UTs

RexXiong pushed a commit that referenced this pull request Oct 30, 2024
…ceFactory

### What changes were proposed in this pull request?
- Added a reset method for DynamicConfigServiceFactory
- Cleaned up QuotaManagerSuite

### Why are the changes needed?
Without this change we can not initialize new configService in any other tests.
Ex: test for this PR #2844 are failing because of this issue.

### Does this PR introduce _any_ user-facing change?
NA

### How was this patch tested?
NA

Closes #2848 from s0nskar/fix_quotatest.

Authored-by: Sanskar Modi <sanskarmodi97@gmail.com>
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
RexXiong pushed a commit that referenced this pull request Oct 30, 2024
…ceFactory

### What changes were proposed in this pull request?
- Added a reset method for DynamicConfigServiceFactory
- Cleaned up QuotaManagerSuite

### Why are the changes needed?
Without this change we can not initialize new configService in any other tests.
Ex: test for this PR #2844 are failing because of this issue.

### Does this PR introduce _any_ user-facing change?
NA

### How was this patch tested?
NA

Closes #2848 from s0nskar/fix_quotatest.

Authored-by: Sanskar Modi <sanskarmodi97@gmail.com>
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
(cherry picked from commit 752a0d9)
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
@s0nskar s0nskar marked this pull request as ready for review October 30, 2024 12:53
@s0nskar
Copy link
Contributor Author

s0nskar commented Nov 5, 2024

cc: @FMX PTAL

@s0nskar
Copy link
Contributor Author

s0nskar commented Nov 7, 2024

@FMX ping on this.

@s0nskar
Copy link
Contributor Author

s0nskar commented Nov 12, 2024

ping @FMX @zwangsheng @mridulm for review

@FMX
Copy link
Contributor

FMX commented Nov 12, 2024

Sorry for the delayed response, I'll review this PR tomorrow.

Copy link
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

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

LGTM except for a nit. There will likely be more PRs to finish this feature. Thanks.

@s0nskar
Copy link
Contributor Author

s0nskar commented Nov 13, 2024

LGTM except for a nit. There will likely be more PRs to finish this feature. Thanks.

@FMX Addressed. Yes, there will be couple more PRs.

@FMX
Copy link
Contributor

FMX commented Nov 14, 2024

Thanks. Merged into main(v0.6.0).

@FMX FMX closed this in a59ebfe Nov 14, 2024
SteNicholas added a commit that referenced this pull request Nov 18, 2024
…wConcurrentHashMap to speed up ConcurrentHashMap#computeIfAbsent

### What changes were proposed in this pull request?

`TagsManager` uses `JavaUtils#newConcurrentHashMap` to speed up `ConcurrentHashMap#computeIfAbsent`.

Follow up #2844.

### Why are the changes needed?

Celeborn supports JDK8, which could meet the bug mentioned in [JDK-8161372](https://bugs.openjdk.org/browse/JDK-8161372). Therefore, it's better to use `JavaUtils#newConcurrentHashMap` to speed up `ConcurrentHashMap#computeIfAbsent`.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

CI.

Closes #2922 from SteNicholas/CELEBORN-1619.

Authored-by: SteNicholas <programgeek@163.com>
Signed-off-by: SteNicholas <programgeek@163.com>
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.

2 participants