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

[pulsar-ml] handle race condition between timeout-task and add-call complete #4455

Merged
merged 2 commits into from
Jun 14, 2019

Conversation

rdhabalia
Copy link
Contributor

Motivation

Broker has addEntry operation-timeout task which can timeout Add-entry operation if it's not completed in configured time. So, addEntry operation can be completed by timeout-task or normal bk-client thread. This PR makes sure that addEntry task can be completed atomically by one thread only.

@rdhabalia rdhabalia added this to the 2.4.0 milestone Jun 3, 2019
@rdhabalia rdhabalia self-assigned this Jun 3, 2019
@rdhabalia
Copy link
Contributor Author

rerun java8 tests

@@ -3080,12 +3080,11 @@ private void checkAddTimeout() {
OpAddEntry opAddEntry = pendingAddEntries.peek();
if (opAddEntry != null) {
boolean isTimedOut = opAddEntry.lastInitTime != -1
&& TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() - opAddEntry.lastInitTime) >= timeoutSec
&& opAddEntry.completed == FALSE;
&& TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() - opAddEntry.lastInitTime) >= timeoutSec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't understand why we removed opAddEntry.completed == FALSE

Copy link
Contributor Author

@rdhabalia rdhabalia Jun 4, 2019

Choose a reason for hiding this comment

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

we need atomic callback handler and want to make sure that OpAddEntry callback completes atomically. So, we replaced boolean completed with addOpCount so, we can update it atomically and only one thread (bk-client/timeout-task) succeed to process the callback.

Copy link
Contributor

@jai1 jai1 left a comment

Choose a reason for hiding this comment

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

Can you add a little more description on how you solve the issue specially on why we changed AtomicIntegerFieldUpdater .

@rdhabalia
Copy link
Contributor Author

rerun java8 tests

@rdhabalia
Copy link
Contributor Author

rerun java8 tests

@codelipenghui
Copy link
Contributor

@merlimat @sijie Please help review this PR if you have time.

@sijie
Copy link
Member

sijie commented Jun 14, 2019

@jai1 can you review it again?

@merlimat merlimat merged commit d72b986 into apache:master Jun 14, 2019
lhotari added a commit to lhotari/pulsar that referenced this pull request May 28, 2021
eolivelli pushed a commit that referenced this pull request May 28, 2021
yangl pushed a commit to yangl/pulsar that referenced this pull request May 29, 2021
wangjialing218 pushed a commit to wangjialing218/pulsar that referenced this pull request May 31, 2021
lhotari added a commit that referenced this pull request Jun 2, 2021
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2021
…completion (apache#10740)

- solution was added in apache#4455

(cherry picked from commit 4372b49)
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 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.

None yet

5 participants