Skip to content

[CELEBORN-1516][FOLLOWUP] Support reset method for DynamicConfigServiceFactory#2848

Closed
s0nskar wants to merge 2 commits intoapache:mainfrom
s0nskar:fix_quotatest
Closed

[CELEBORN-1516][FOLLOWUP] Support reset method for DynamicConfigServiceFactory#2848
s0nskar wants to merge 2 commits intoapache:mainfrom
s0nskar:fix_quotatest

Conversation

@s0nskar
Copy link
Contributor

@s0nskar s0nskar commented Oct 24, 2024

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

@s0nskar s0nskar changed the title Adding reset method for DynamicConfigServiceFactory [CELEBORN-1516][FOLLOWUP] Support reset method for DynamicConfigServiceFactory Oct 24, 2024
@s0nskar s0nskar marked this pull request as ready for review October 24, 2024 08:18
@s0nskar
Copy link
Contributor Author

s0nskar commented Oct 24, 2024

cc: @SteNicholas @leixm PTAL

@s0nskar
Copy link
Contributor Author

s0nskar commented Oct 28, 2024

bumping this up^ @FMX @RexXiong


@VisibleForTesting
public static void reset() {
_INSTANCE = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need call shutdown method before reset the _INSTANCE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RexXiong updated the PR

Copy link
Contributor

@RexXiong RexXiong left a comment

Choose a reason for hiding this comment

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

LGTM

@s0nskar
Copy link
Contributor Author

s0nskar commented Oct 30, 2024

Can we please merge this, this is blocking another PR - #2844

@RexXiong
Copy link
Contributor

RexXiong commented Oct 30, 2024

Can we please merge this, this is blocking another PR - #2844

Sure, merge to main(v0.6.0) and branch-0.5(v0.5.3)

@RexXiong RexXiong closed this in 752a0d9 Oct 30, 2024
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
Copy link
Contributor Author

s0nskar commented Oct 30, 2024

Thanks @RexXiong.

@s0nskar s0nskar deleted the fix_quotatest branch October 30, 2024 10:55
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