KAFKA-4547: Avoid unnecessary offset commit that could lead to an invalid offset position if partition is paused by vahidhashemian · Pull Request #2341 · apache/kafka

vahidhashemian · 2017-01-10T21:46:07Z

No description provided.

asfbot · 2017-01-10T22:27:27Z

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

asfbot · 2017-01-10T22:33:44Z

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

asfbot · 2017-01-10T23:51:03Z

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

vahidhashemian · 2017-01-10T23:51:30Z

@hachikuji I submitted this PR without test(s) in case it needs to be included in 0.10.2.0. I'll work on creating some tests after I'm done with the PR for KIP-88.

hachikuji · 2017-01-11T00:59:29Z

Can we use "has" for consistency?

Sure, I'll update it.

hachikuji · 2017-01-11T01:01:05Z

"for the given partitions"?

asfbot · 2017-01-11T02:17:45Z

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

asfbot · 2017-01-11T02:18:10Z

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

asfbot · 2017-01-11T04:00:56Z

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

hachikuji · 2017-01-11T18:12:47Z

Seems we could simplify this by delegating to the other method.

Or maybe we don't even need this method anymore (haven't checked)

It's being used in a couple of place: here and here.

Even though they can be modified to leverage the new method, I'd prefer to keep it and just modify its implementation to delegate to the other one as you suggested. What do you think?

I submitted an update that refactors the original method. I'd be happy to take your second suggestion too if you think that's a better way to go. Thanks.

If it's used, we can keep it. I wasn't sure, so thought I'd mention it.

asfbot · 2017-01-11T19:19:11Z

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

asfbot · 2017-01-11T19:19:57Z

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

asfbot · 2017-01-11T21:37:36Z

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

asfbot · 2017-01-14T01:25:32Z

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

asfbot · 2017-01-14T01:40:41Z

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

asfbot · 2017-01-14T01:47:59Z

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

ewencp · 2017-01-16T05:25:53Z

@hachikuji @vahidhashemian Please note that I've updated the affected versions to 0.10.0.2 and 0.10.1.0, 0.10.1.1. When we merge a fix, we want to make sure it makes it onto branches 0.10.0, 0.10.1, 0.10.2, and trunk.

hachikuji · 2017-01-17T19:26:23Z

@vahidhashemian The fix looks good, but can you add a couple test cases?

asfbot · 2017-01-19T02:24:11Z

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

asfbot · 2017-01-19T02:24:16Z

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

asfbot · 2017-01-19T02:25:11Z

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

hachikuji · 2017-01-19T03:24:58Z

Thanks for adding the test. Would be good to have a more focused test in FetcherTest as well for the change there.

Sure, I'll try to add that too. Thanks for the quick feedback.

asfbot · 2017-01-19T03:31:18Z

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

asfbot · 2017-01-19T03:32:10Z

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

asfbot · 2017-01-19T03:37:56Z

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

ijuma · 2017-01-19T16:44:10Z

@ewencp does it affect 0.10.0.2 as well? The reporter suggested that 0.10.0.1 worked fine while 0.10.1.0 did not, so I was wondering if you had additional information than what was in the JIRA.

…alid offset position if partition is paused

asfbot · 2017-01-19T20:48:23Z

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

asfbot · 2017-01-19T20:48:53Z

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

asfbot · 2017-01-19T20:50:22Z

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

hachikuji · 2017-01-19T21:01:36Z

Could we have one more test case similar to this, but in which the paused partition already has a position? This verifies that updateFetchPositions does not overwrite the current position.

Sure. I'll add one more. Thanks.

hachikuji · 2017-01-19T22:08:46Z

nit: if you use Utils.mkSet, then you won't need the conversion below.

asfbot · 2017-01-19T22:58:41Z

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

asfbot · 2017-01-19T23:07:28Z

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

asfbot · 2017-01-19T23:59:43Z

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

hachikuji · 2017-01-20T00:24:47Z

LGTM. I think @ijuma is right that this only needs to go into 0.10.1 and 0.10.2.

…ed offsets Author: Vahid Hashemian <vahidhashemian@us.ibm.com> Reviewers: Jason Gustafson <jason@confluent.io> Closes #2341 from vahidhashemian/KAFKA-4547

hachikuji · 2017-01-20T01:01:10Z

@vahidhashemian Merged to trunk and 0.10.2. Unfortunately, it doesn't merge cleanly onto 0.10.1. Would you mind submitting a separate PR to that branch?

vahidhashemian · 2017-01-20T06:00:45Z

@hachikuji Definitely.

…ed offsets Author: Vahid Hashemian <vahidhashemian@us.ibm.com> Reviewers: Jason Gustafson <jason@confluent.io> Closes apache#2341 from vahidhashemian/KAFKA-4547

hachikuji reviewed Jan 11, 2017

View reviewed changes

vahidhashemian force-pushed the KAFKA-4547 branch from e88617d to 2330bcf Compare January 11, 2017 01:36

hachikuji reviewed Jan 11, 2017

View reviewed changes

vahidhashemian force-pushed the KAFKA-4547 branch from 2330bcf to d95ff3b Compare January 11, 2017 18:36

vahidhashemian force-pushed the KAFKA-4547 branch from d95ff3b to ffdb19c Compare January 14, 2017 00:51

vahidhashemian force-pushed the KAFKA-4547 branch from ffdb19c to 0426eab Compare January 19, 2017 02:21

vahidhashemian force-pushed the KAFKA-4547 branch from 0426eab to d040ff7 Compare January 19, 2017 02:42

hachikuji reviewed Jan 19, 2017

View reviewed changes

vahidhashemian added 2 commits January 19, 2017 11:57

KAFKA-4547: Avoid unnecessary offset commit that could lead to an inv…

1a2e947

…alid offset position if partition is paused

Added a unit test

4cbdff3

vahidhashemian force-pushed the KAFKA-4547 branch from d040ff7 to ea08690 Compare January 19, 2017 20:01

hachikuji reviewed Jan 19, 2017

View reviewed changes

vahidhashemian force-pushed the KAFKA-4547 branch 2 times, most recently from f2540d5 to fbdb44d Compare January 19, 2017 21:58

hachikuji reviewed Jan 19, 2017

View reviewed changes

Added unit tests in FetcherTest to cover paused partitions

81dfc01

vahidhashemian force-pushed the KAFKA-4547 branch from fbdb44d to 81dfc01 Compare January 19, 2017 22:17

asfgit closed this in 813897a Jan 20, 2017

Conversation

vahidhashemian commented Jan 10, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

vahidhashemian commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vahidhashemian Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

asfbot commented Jan 11, 2017

Uh oh!

asfbot commented Jan 14, 2017

Uh oh!

asfbot commented Jan 14, 2017

Uh oh!

asfbot commented Jan 14, 2017

Uh oh!

ewencp commented Jan 16, 2017

Uh oh!

hachikuji commented Jan 17, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

ijuma commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

asfbot commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vahidhashemian Jan 11, 2017 •

edited

Loading