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] [test] Fix flaky test MetadataStoreStatsTest.testBatchMetadataStoreMetrics #19793

Merged

Conversation

lifepuzzlefun
Copy link
Contributor

@lifepuzzlefun lifepuzzlefun commented Mar 11, 2023

Fixes #19792

Motivation

Fix flaky test MetadataStoreStatsTest.testBatchMetadataStoreMetrics

some unit test create MetadataStore without setting name and not close will cause prometheus metric not unregisterd.
the unittest above will check promethues metric tag name not null.

Modifications

  1. set metadata store name to avoid generate metric with name cause relate unit test fail.

  2. auto generate metadata store name if caller not specify one for avoid this problem occur again.

Verifying this change

This change is already covered by existing tests, such as (please describe 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:

1. set metadata store name to avoid generate metric with name cause relate unit test fail.

2. auto generate metadata store name if caller not specify one for avoid this problem occur again.
@github-actions
Copy link

@lifepuzzlefun Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 11, 2023
@lifepuzzlefun
Copy link
Contributor Author

lifepuzzlefun commented Mar 11, 2023

the auto generate metadata store name will be like org.apache.pulsar.metadata.MetadataStoreConfigTest_testGeneCallerStackNameWhenMetadataStoreNameNotSet_11

I made some change in my local repo to capture the caller lifepuzzlefun#10

and see CI job fail message which wil print stack trace if check metric tag with name empty and make unit test fail.
https://github.com/lifepuzzlefun/pulsar/actions/runs/4391908015/jobs/7691586022

@nodece
Copy link
Member

nodece commented Mar 13, 2023

Is the root cause that the metadatastoreName is empty?

@lifepuzzlefun
Copy link
Contributor Author

Is the root cause that the metadatastoreName is empty?

@nodece The root cause is create one MetadataStore with no name set and not close it.

@Override
public MetadataStoreConfig build() {
if (Strings.isEmpty(this.callerSetMetadataStoreName)) {
Exception e = new Exception("stack trace");
Copy link
Member

Choose a reason for hiding this comment

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

I think you can throw an exception, so like:

throw new IllegalArgumentException("metadataStoreName is required");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. the problem is there is a lot of ut didn't set name. If they don't set name but they close the object the flaky test won't occur.

Copy link
Member

Choose a reason for hiding this comment

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

So can we just need to close the metadatastore to fix the flaky test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, other modification is for this case not happen again and supply some method to find the wrong usage.

the modification won't hurt production.

Copy link
Member

Choose a reason for hiding this comment

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

yes, other modification is for this case not happen again and supply some method to find the wrong usage.

Makes sense! But this is rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i agree with you. maybe we can add one comment in this unit test point to the current modification to catch up new wrong usage. and the branch will be retain for those who is block by this problem.

I'll remove the current metadataStore name generate code for this patch to merge.

@lifepuzzlefun
Copy link
Contributor Author

@nodece @coderzc please take a look.

@nodece
Copy link
Member

nodece commented Mar 15, 2023

/pulsarbot rerun-failure-checks

@nodece nodece requested a review from lhotari March 15, 2023 02:35
@nodece
Copy link
Member

nodece commented Mar 15, 2023

/pulsarbot rerun-failure-checks

@nodece nodece merged commit 4e48cc3 into apache:master Mar 15, 2023
@nodece
Copy link
Member

nodece commented Mar 15, 2023

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

Successfully merging this pull request may close these issues.

Flaky-test: org.apache.pulsar.broker.stats.MetadataStoreStatsTest.testBatchMetadataStoreMetrics
4 participants