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

Revert "Fix: LockManagerTest.updateValue is flaky (#13911)" #15235

Merged
merged 1 commit into from
May 7, 2022

Conversation

shibd
Copy link
Member

@shibd shibd commented Apr 20, 2022

Motivation

#15091

The essential reason for #13911 is that two threads execute the refresh method concurrently. In PR #13911 change, it doesn't completely solve the problem, just avoid concurrent access by threads.

In #14283, The problem was radically resolved by changing below.

objCache.asMap().computeIfPresent(path, (oldKey, oldValue) -> readValueFromStore(path));

In #13911, although multi-threaded access is avoided, it will cause the test future.get() to not get the expected result.
This will cause the updateValueWhenKeyDisappears test to fail. So, revert this commit.

Modifications

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 20, 2022
@codelipenghui
Copy link
Contributor

@shibd So we need another solution for the flaky test? this PR just revert the previous solution?

@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 21, 2022
@shibd
Copy link
Member Author

shibd commented Apr 21, 2022

@shibd So we need another solution for the flaky test? this PR just revert the previous solution?

No need, Because #14283 also solved these flaky tests.

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

Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mattisonchao mattisonchao merged commit 306a0a1 into apache:master May 7, 2022
codelipenghui pushed a commit that referenced this pull request May 20, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
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.

6 participants