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-4942 fixed the commitTimeoutMs being set before the commit actually started #2730

Closed

Conversation

@simplesteph
Copy link
Contributor

commented Mar 24, 2017

this fixes KAFKA-4942

@asfbot

This comment has been minimized.

Copy link

commented Mar 24, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2365/
Test FAILed (JDK 8 and Scala 2.11).

@simplesteph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2017

unsure why the CI is failing, but the tests are passing on my laptop

@asfbot

This comment has been minimized.

Copy link

commented Mar 24, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2361/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot

This comment has been minimized.

Copy link

commented Mar 24, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2361/
Test PASSed (JDK 8 and Scala 2.12).

@gwenshap

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

@ewencp or @kkonstantine: mind checking? this looks like a potentially important fix.

@ewencp

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

@simplesteph Good catch, fix makes sense given the call to commitOffsets in there.

It seems that this was introduced as a regression in KAFKA-4161: KIP-89: Allow sink connectors to decouple flush and offset commit. Perhaps there is a unit test in WorkerSinkTaskTest that could catch such a regression?

@simplesteph

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2017

@ewencp I've tried looking at writing a test for over an hour but no success as I'm not familiar with the PowerMock / EasyMock libraries. I also have no idea how to trigger two commits in a row, including a long commit that would keep the committing flag to true (it goes to false right away no matter what I do, therefore never giving me the chance to get the timestamp comparison error). Anyway, I'd love to see making it part of 0.10.2.1, but I'm afraid I won't be able to provide a test in time, if at all. I'm happy for anyone to take ownership of my 1 line of work if they're able to write a test. Sorry I couldn't be more helpful

@56quarters

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

Hello,

I believe I've added a test that verifies that the change by @simplesteph is correct. I've confirmed that without his fix, this test does not pass.

Disclaimer: The test is not exactly pretty. It required spinning up an ExecutorService to replicate a long running commit.

The test is available here if someone would like to merge it into this work: smarter-travel-media@9569a04

Alternatively, I can open a new PR with this commit and @simplesteph's work if that's easier.

Please let me know if there's anything you'd like me to do or change. Thanks!

@simplesteph

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2017

HI @56quarters . Feel free to open a PR and include my one line change :) You did most of the work, I really appreciate it

@asfbot

This comment has been minimized.

Copy link

commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3151/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot

This comment has been minimized.

Copy link

commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3150/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot

This comment has been minimized.

Copy link

commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3156/
Test PASSed (JDK 8 and Scala 2.11).

asfgit pushed a commit that referenced this pull request Jun 6, 2017
KAFKA-4942: Fix commitTimeoutMs being set before the commit actually …
…started

This fixes KAFKA-4942

This supersededs #2730

/cc simplesteph gwenshap ewencp

Author: Nick Pillitteri <nickp@smartertravelmedia.com>
Author: simplesteph <stephane.maarek@gmail.com>

Reviewers: simplesteph <stephane.maarek@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #2912 from 56quarters/fix-connect-offset-commit

(cherry picked from commit d655d80)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
asfgit pushed a commit that referenced this pull request Jun 6, 2017
KAFKA-4942: Fix commitTimeoutMs being set before the commit actually …
…started

This fixes KAFKA-4942

This supersededs #2730

/cc simplesteph gwenshap ewencp

Author: Nick Pillitteri <nickp@smartertravelmedia.com>
Author: simplesteph <stephane.maarek@gmail.com>

Reviewers: simplesteph <stephane.maarek@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #2912 from 56quarters/fix-connect-offset-commit
@ewencp

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2017

@simplesteph Could you close this one out since #2912 has been committed?

@simplesteph

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2017

@ewencp done. Thanks for merging the other one. Badly needed fix :)

@simplesteph simplesteph closed this Jun 6, 2017

@asfbot

This comment has been minimized.

Copy link

commented Jun 6, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4966/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot

This comment has been minimized.

Copy link

commented Jun 6, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4982/
Test PASSed (JDK 7 and Scala 2.11).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.