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.

Sign up for GitHub

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

Jump to bottom

KAFKA-3960 - Committed offset not set after first assign #1629

Closed
wants to merge 1 commit into from
Closed

KAFKA-3960 - Committed offset not set after first assign #1629

wants to merge 1 commit into from

Conversation

13h3r
Copy link
Contributor

@13h3r 13h3r commented Jul 18, 2016 •
edited

No description provided.

@ijuma
Copy link
Contributor

ijuma commented Jul 18, 2016

cc @hachikuji

@13h3r
Copy link
Contributor Author

13h3r commented Jul 18, 2016

Could now figure out why build failed. Any advise?

@hachikuji
Copy link
Contributor

Nice find @13h3r. Would you mind adding a test case? The jenkins failure seems unrelated.

@13h3r
Copy link
Contributor Author

13h3r commented Jul 19, 2016

Done

@13h3r
Copy link
Contributor Author

13h3r commented Jul 20, 2016

Anything else I have to do to get this merged?

@ijuma
Copy link
Contributor

ijuma commented Jul 20, 2016 •
edited

@13h3r, I think it would be nice to have a test that exercised the consumer to complement the SubscriptionState unit test (to make it less likely that we break this again if we refactor things). Also, it seems like the PR description is just restating the PR title, I would edit it and make it blank if there's nothing else to add (the PR description becomes the merged commit message so that's why we care).

@13h3r
Copy link
Contributor Author

13h3r commented Jul 21, 2016

@ijuma Thanks for the advise. Consumer test implemented.

hachikuji
hachikuji reviewed Jul 21, 2016
View reviewed changes
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
int sessionTimeoutMs = 3000;
int heartbeatIntervalMs = 2000;

// adjust auto commit interval lower than heartbeat so we don't need to deal with
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this comment is not relevant for this test case

@hachikuji
Copy link
Contributor

Minor comment, but otherwise LGTM.

ijuma
ijuma reviewed Jul 21, 2016
View reviewed changes
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
long offset1 = 10000;
long offset2 = 20000;


Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need these two empty lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I think I'd remove all empty lines all the way to autoCommitIntervalMs as I don't think they add much.

@13h3r
Copy link
Contributor Author

13h3r commented Jul 23, 2016

Is it possible to rerun tests? Everything is fine on my laptop

@ijuma
Copy link
Contributor

ijuma commented Jul 23, 2016

I ran the tests locally and they pass. Thanks for the PR, LGTM. Merged to trunk and 0.10.0 branches.

@asfgit asfgit closed this in 932bb84 Jul 23, 2016
asfgit pushed a commit that referenced this pull request Jul 23, 2016
@13h3r @ijuma
KAFKA-3960; Committed offset not set after first assign
1791445 
Author: Alexey Romanchuk <al.romanchuk@2gis.ru>

Reviewers: Jason Gustafson <jason@confluent.io>, Ismael Juma <ismael@juma.me.uk>

Closes #1629 from 13h3r/kafka-3960

(cherry picked from commit 932bb84)
Signed-off-by: Ismael Juma <ismael@juma.me.uk>
granthenke pushed a commit to granthenke/kafka that referenced this pull request Oct 24, 2016
@13h3r
KAFKA-3960; Committed offset not set after first assign
d9b584d 
Author: Alexey Romanchuk <al.romanchuk@2gis.ru>

Reviewers: Jason Gustafson <jason@confluent.io>, Ismael Juma <ismael@juma.me.uk>

Closes apache#1629 from 13h3r/kafka-3960

(cherry picked from commit 932bb84)
Signed-off-by: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@13h3r @ijuma @hachikuji