Skip to content

[fix] [test] Fix flaky test MetadataStoreStatsTest.testBatchMetadataStoreMetrics#19793

Merged
nodece merged 10 commits into
apache:masterfrom
lifepuzzlefun:fix_flacky_test_metadadata_store_not_set_name
Mar 15, 2023
Merged

[fix] [test] Fix flaky test MetadataStoreStatsTest.testBatchMetadataStoreMetrics#19793
nodece merged 10 commits into
apache:masterfrom
lifepuzzlefun:fix_flacky_test_metadadata_store_not_set_name

Conversation

@lifepuzzlefun

@lifepuzzlefun lifepuzzlefun commented Mar 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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:

@lifepuzzlefun
fix flaky test MetadataStoreStatsTest.testBatchMetadataStoreMetrics
4e66eae 
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

github-actions Bot commented Mar 11, 2023

Copy link
Copy Markdown

@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

lifepuzzlefun commented Mar 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@lifepuzzlefun lifepuzzlefun mentioned this pull request Mar 11, 2023
Merged
14 tasks
@nodece

nodece commented Mar 13, 2023

Copy link
Copy Markdown
Member

Is the root cause that the metadatastoreName is empty?

@lifepuzzlefun

lifepuzzlefun commented Mar 13, 2023

Copy link
Copy Markdown
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.

nodece
nodece reviewed Mar 14, 2023
View reviewed changes
Comment thread pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataStoreConfig.java Outdated
@Override
public MetadataStoreConfig build() {
if (Strings.isEmpty(this.callerSetMetadataStoreName)) {
Exception e = new Exception("stack trace");

@nodece nodece Mar 14, 2023

Copy link
Copy Markdown
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");

@lifepuzzlefun lifepuzzlefun Mar 14, 2023

Copy link
Copy Markdown
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.

@nodece nodece Mar 14, 2023

Copy link
Copy Markdown
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?

@lifepuzzlefun lifepuzzlefun Mar 14, 2023

Copy link
Copy Markdown
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.

@nodece nodece Mar 14, 2023

Copy link
Copy Markdown
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.

@lifepuzzlefun lifepuzzlefun Mar 14, 2023

Copy link
Copy Markdown
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.

@nodece nodece added this to the 3.0.0 milestone Mar 14, 2023
@lifepuzzlefun

lifepuzzlefun commented Mar 14, 2023

Copy link
Copy Markdown
Contributor Author

@nodece @coderzc please take a look.

nodece
nodece reviewed Mar 14, 2023
View reviewed changes
Comment thread pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/MetadataStoreStatsTest.java Outdated
@nodece

nodece commented Mar 15, 2023

Copy link
Copy Markdown
Member

/pulsarbot rerun-failure-checks

@nodece nodece requested a review from lhotari March 15, 2023 02:35
@dao-jun dao-jun mentioned this pull request Mar 15, 2023
Merged
4 tasks
@nodece

nodece commented Mar 15, 2023

Copy link
Copy Markdown
Member

/pulsarbot rerun-failure-checks

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

nodece commented Mar 15, 2023

Copy link
Copy Markdown
Member

This fix is invalid, see https://github.com/apache/pulsar/actions/runs/4424020709/jobs/7763604452#step:11:1277

@yyj8 yyj8 mentioned this pull request Mar 16, 2023
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nodece nodece nodece approved these changes

@coderzc coderzc coderzc approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

Assignees

@lifepuzzlefun lifepuzzlefun

Labels

area/test doc-not-needed Your PR changes do not impact docs ready-to-test type/flaky-tests

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

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

4 participants

@lifepuzzlefun @nodece @coderzc @lifepuzzlefun1