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

KAFKA-2799: skip wakeup in the follow-up poll() call. #490

Closed
wants to merge 2 commits into from

Conversation

@guozhangwang
Copy link
Contributor

commented Nov 10, 2015

No description provided.

@guozhangwang

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

Ping @hachikuji for reviews.

v1
this.client.wakeup();
}
this.wakeup.set(true);
this.client.wakeup();

This comment has been minimized.

Copy link
@hachikuji

hachikuji Nov 10, 2015

Contributor

I'm wondering if we also need to delay the call to client.wakeup()? For example, consider the following sequence:

  1. disableWakeups()
  2. wakeup()
  3. poll(0)
  4. enableWakeup()
  5. poll(Long.MAX_VALUE)

The underlying wakeup on Selector would effect the poll(0), but we would suppress the exception. Then there would be nothing to wake us up from the poll(Long.MAX_VALUE).

This comment has been minimized.

Copy link
@guozhangwang

guozhangwang Nov 10, 2015

Author Contributor

poll(0) will effectively trigger the underlying Selector.selectNow, which is not a blocking call and hence will not be affected by the Selector.wakeup call; instead that call will affect the poll(Long.MAX_VALUE) causing it to return immediately:

http://docs.oracle.com/javase/7/docs/api/java/nio/channels/Selector.html#wakeup()

This comment has been minimized.

Copy link
@hachikuji

hachikuji Nov 10, 2015

Contributor

"If no selection operation is currently in progress then the next invocation of one of these methods will return immediately unless the selectNow() method is invoked in the meantime."

I read that to mean that that poll(Long.MAX_VALUE) will not return immediately since a selectNow was invoked in the meantime . Might be worth a unit test to verify that it works as expected.

This comment has been minimized.

Copy link
@guozhangwang

guozhangwang Nov 10, 2015

Author Contributor

I am with you that selectNow will clear the previous wakeup call, as stated "Invoking this method clears the effect of any previous invocations of the wakeup method."

What I thought is, assuming selectNow and wakeup are atomic (which they should since they are not blocking). Then we could have the following traces:

this.wakeup.set(true);    // in wakeup()
this.client.wakeup();       // in wakeup()
if (wakeup.get())...          // in poll()
this.client.poll(0);   // in poll()

and hence it will throw an exception in step 3) without going to step 4). Or in another case:

if (wakeup.get())...          // in poll()
this.wakeup.set(true);    // in wakeup()
this.client.poll(0);   // in poll()
this.client.wakeup();       // in wakeup()

Then the wakeup call in step 4) will be maintained for the next poll. And:

if (wakeup.get())...          // in poll()
this.wakeup.set(true);    // in wakeup()
this.client.wakeup();       // in wakeup()
this.client.poll(0);   // in poll()

This is the problem, and even delaying the wakeup will not help here.

@guozhangwang

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

Comments addressed, also made wakeupEnabled flag volatile.

@hachikuji

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2015

LGTM

@asfgit asfgit closed this in 9d9bb70 Nov 10, 2015

guozhangwang added a commit to confluentinc/kafka that referenced this pull request Nov 18, 2015
KAFKA-2799: skip wakeup in the follow-up poll() call.
Author: Guozhang Wang <wangguoz@gmail.com>

Reviewers: Jason Gustafson

Closes apache#490 from guozhangwang/K2799
asfgit pushed a commit that referenced this pull request Nov 25, 2015
KAFKA-2799: skip wakeup in the follow-up poll() call.
Cherry-picked from trunk.

Author: Guozhang Wang <wangguoz@gmail.com>

Reviewers: Jason Gustafson

Closes #490 from guozhangwang/K2799

@guozhangwang guozhangwang deleted the guozhangwang:K2799 branch Oct 7, 2016

gwenshap added a commit to confluentinc/kafka that referenced this pull request Jul 21, 2019
CONFLUENT: Jenkinsfile should not swallow test return status codes (a…
…pache#490)

Our jenkins setup has been swallowing exits when running unit and integration tests. When JUnit test failures are not correctly captured, this can cause our builds to pass when they should have failed, as well as hiding cases where gradle forks exit with status code 1.

-- This change also lowers the max forks to 4, to help stabilize builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.