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][flaky-test] Fix and improve LookupRetryTest #17848

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Sep 27, 2022

Fixes #17785

Motivation

The failureMap need to be clear after run per unit test.

Modifications

Clear failureMap after run per unit test, and only run once setup()/cleanup() to reduce execution time.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: coderzc#6

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 27, 2022
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Nice catch!

@tisonkun
Copy link
Member

@coderzc although, can you explain a bit about how this patch fix the original issue?

The original failure reports:

  Error:  testCloseConnectionOnBrokerRejectedRequest(org.apache.pulsar.client.impl.LookupRetryTest)  Time elapsed: 2.146 s  <<< FAILURE!
  java.lang.AssertionError: expected [2] but found [1]
  	at org.testng.Assert.fail(Assert.java:99)
  	at org.testng.Assert.failNotEquals(Assert.java:1037)
  	at org.testng.Assert.assertEqualsImpl(Assert.java:140)
  	at org.testng.Assert.assertEquals(Assert.java:122)
  	at org.testng.Assert.assertEquals(Assert.java:907)
  	at org.testng.Assert.assertEquals(Assert.java:917)
  	at org.apache.pulsar.client.impl.LookupRetryTest.testCloseConnectionOnBrokerRejectedRequest(LookupRetryTest.java:217)

That doesn't directly related to the hash map.

@coderzc
Copy link
Member Author

coderzc commented Sep 27, 2022

@coderzc although, can you explain a bit about how this patch fix the original issue?

The original failure reports:

  Error:  testCloseConnectionOnBrokerRejectedRequest(org.apache.pulsar.client.impl.LookupRetryTest)  Time elapsed: 2.146 s  <<< FAILURE!
  java.lang.AssertionError: expected [2] but found [1]
  	at org.testng.Assert.fail(Assert.java:99)
  	at org.testng.Assert.failNotEquals(Assert.java:1037)
  	at org.testng.Assert.assertEqualsImpl(Assert.java:140)
  	at org.testng.Assert.assertEquals(Assert.java:122)
  	at org.testng.Assert.assertEquals(Assert.java:907)
  	at org.testng.Assert.assertEquals(Assert.java:917)
  	at org.apache.pulsar.client.impl.LookupRetryTest.testCloseConnectionOnBrokerRejectedRequest(LookupRetryTest.java:217)

That doesn't directly related to the hash map.

Oh, I haven't found the root cause of the original case for the time being, but when I have repeatedly executed testCloseConnectionOnBrokerRejectedRequest many times, if don‘t clearfailureMap will cause similar problems.

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@tisonkun
Copy link
Member

/pulsarbot run-failure-checks

@coderzc
Copy link
Member Author

coderzc commented Sep 28, 2022

/pulsarbot run-failure-checks

@congbobo184 congbobo184 merged commit 31203c3 into apache:master Sep 28, 2022
@coderzc coderzc changed the title [fix][flask-test] Fix and improve LookupRetryTest [fix][flaky-test] Fix and improve LookupRetryTest Nov 1, 2022
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 ready-to-test type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: LookupRetryTest.testCloseConnectionOnBrokerRejectedRequest
5 participants