Repository navigation
Conversation
|
Thanks for finding this and providing a fix. Do you think you'd be able to write a test for this? If not, we can try and help. |
|
I am currently trying to write a test, but it will take some time, since I am not very familiar with Easymock and the kafka code. |
|
Refer to this link for build results (access rights to CI server needed): |
|
here is the test |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the test. I left some minor comments, but looks good overall. It will be good for @hachikuji to review this as well.
There was a problem hiding this comment.
Can you please document what this method returns?
There was a problem hiding this comment.
What happened here without the fix?
There was a problem hiding this comment.
is this a question to answer here or to be documented in code?
without the fix, another leader would be elected ("a"), the member Set is different(Set("a") and the offset is 23L
There was a problem hiding this comment.
It was just for my own benefit, thanks.
There was a problem hiding this comment.
It might be nice to include more partitions in the segments to help catch other regressions in the future. We could, for example, have 2 partitions in segment 1 and 3 partitions in segment 2.
…d log correctly the while loop was too big and need to be closed earlier to see the fix, ignore whitespace since most of it is indentation this bug was introduced by commit 5bd06f1
|
Refer to this link for build results (access rights to CI server needed): |
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Thanks for catching this. Just one clarification. It seems the bug is not actually dependent on reading from multiple segments. Multiple reads within the same segment could also cause the issue (which I'm guessing is this line: https://github.com/apache/kafka/pull/3538/files#diff-47448559499d691a7c286fa29d5b202dR600).
There was a problem hiding this comment.
of course, but it is more persistent and reproducable on the segment boundary
There was a problem hiding this comment.
That seems debatable since the cleaner will kick in for older segments. In any case, if the underlying issue is more general, we should probably update the JIRA and PR titles.
There was a problem hiding this comment.
I don't want to debate, I want this to be fixed, because we have this issue on some of our systems. I will change issue title, commit message and PR title tomorow morning, but I think it is too complicated to reproduce the single segment multiple read case (unit test case is the same).
There was a problem hiding this comment.
We can reword the title when we merge if that is acceptable. I just want to make the issue clear for users who encounter it.
|
Refer to this link for build results (access rights to CI server needed): |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. I will merge to trunk and 0.11.0 with a more general commit title.
…t cache the while loop was too big and need to be closed earlier to see the fix, ignore whitespace since most of it is indentation this bug was introduced by commit 5bd06f1 Author: Jan Burkhardt <[email protected]> Reviewers: Ismael Juma <[email protected]>, Jason Gustafson <[email protected]> Closes #3538 from bjrke/trunk (cherry picked from commit e2fe19d) Signed-off-by: Jason Gustafson <[email protected]>
4 participants
…d log correctly
the while loop was too big and need to be closed earlier
to see the fix, ignore whitespace since most of it is indentation
this bug was introduced by commit
5bd06f1