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

[fix][proxy] Cleanup default collector registry when closing the ProxyServiceStarter #19062

Conversation

gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Dec 26, 2022

Fixes: #19011

Motivation

Related issue #19011.

Most of the tests are all based on the class TestRetrySupport, which supports resetting the broken test environments, the ProxyServiceStarter will register metrics in the default collector registry if we restart the ProxyServiceStarter, it will throw the exception like this.

java.lang.IllegalArgumentException: Failed to register Collector of type Gauge: jvm_memory_direct_bytes_used is already in use by another Collector of type Gauge

Modifications

Clean up the default collector registry when closing the ProxyServiceStarter.

Verifying this change

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: gaoran10#20

Cleanup collector registry when closing the ProxyServiceStarter in test.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 26, 2022
@gaoran10 gaoran10 self-assigned this Dec 26, 2022
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2022

Codecov Report

Merging #19062 (9745033) into master (b42aed1) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19062      +/-   ##
============================================
- Coverage     47.11%   46.90%   -0.22%     
+ Complexity    10595    10588       -7     
============================================
  Files           710      710              
  Lines         69423    69424       +1     
  Branches       7449     7449              
============================================
- Hits          32709    32563     -146     
- Misses        33037    33228     +191     
+ Partials       3677     3633      -44     
Flag Coverage Δ
unittests 46.90% <100.00%> (-0.22%) ⬇️

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

Impacted Files Coverage Δ
...pache/pulsar/proxy/server/ProxyServiceStarter.java 60.92% <100.00%> (+0.26%) ⬆️
...rvice/schema/KeyValueSchemaCompatibilityCheck.java 21.62% <0.00%> (-45.95%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 10.40% <0.00%> (-30.40%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.27% <0.00%> (-27.05%) ⬇️
...g/apache/bookkeeper/mledger/util/StatsBuckets.java 43.75% <0.00%> (-16.67%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 60.20% <0.00%> (-11.74%) ⬇️
...ookkeeper/mledger/impl/ManagedLedgerMBeanImpl.java 53.17% <0.00%> (-9.53%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 42.02% <0.00%> (-9.44%) ⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java 58.33% <0.00%> (-8.34%) ⬇️
...ker/resourcegroup/ResourceGroupPublishLimiter.java 31.03% <0.00%> (-8.05%) ⬇️
... and 33 more

@codelipenghui codelipenghui merged commit b24034f into apache:master Dec 27, 2022
tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Dec 27, 2022
@lhotari
Copy link
Member

lhotari commented Jan 17, 2023

This change has to be reverted or revisited since it's causing a lot of build failures. The issue is #19216.

Counters registered in static field initializers such as

static final Counter BYTES_COUNTER = Counter
.build("pulsar_proxy_binary_bytes", "Counter of proxy bytes").create().register();
won't get re-registered and that's why this change makes another test break.

@lhotari
Copy link
Member

lhotari commented Jan 17, 2023

I'm working on a revisited change.

lhotari added a commit to lhotari/pulsar that referenced this pull request Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: ProxyServiceStarterDisableZeroCopyTest.stateCheck
5 participants