Skip to content

MINOR: Clean up of ConsumerCoordinator and PartitionAssignor.#1306

Closed
Ishiihara wants to merge 2 commits into
apache:trunkfrom
Ishiihara:minor-consumer-cleanup
Closed

MINOR: Clean up of ConsumerCoordinator and PartitionAssignor.#1306
Ishiihara wants to merge 2 commits into
apache:trunkfrom
Ishiihara:minor-consumer-cleanup

Conversation

@Ishiihara

Copy link
Copy Markdown
Contributor

No description provided.

@Ishiihara

Copy link
Copy Markdown
Contributor Author

@hachikuji Not a big deal. Just some clean up.

@ijuma

ijuma commented May 3, 2016

Copy link
Copy Markdown
Member

The clean-ups are fine, but neither of the two files changed are KafkaConsumer.

@granthenke

Copy link
Copy Markdown
Member

@ijuma It is Kafka Consumer internals.
@Ishiihara Perhaps adding Internals to the PR title will make it more clear.

@ijuma

ijuma commented May 3, 2016

Copy link
Copy Markdown
Member

@granthenke I know that (obviously). :) I was suggesting that we can make the PR title (and hence commit message) clearer. Something like MINOR: Clean up of ConsumerCoordinator and PartitonAssignor

import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Collection;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checkstyle doesn't like this import since it's only used in the javadoc. You might need to use the full name in the javadoc instead.

@Ishiihara Ishiihara changed the title MINOR: Clean up of Kafka Consumer MINOR: Clean up of ConsumerCoordinator and PartitionAssigner. May 3, 2016
@Ishiihara

Copy link
Copy Markdown
Contributor Author

@ijuma @granthenke Thanks for the review and suggestion on making the tile more clear.
@hachikuji Thanks for pointing me the issue with checkstyle.

@Ishiihara

Copy link
Copy Markdown
Contributor Author

@ijuma @granthenke @hachikuji Addressed the review comments. Would you mind to take another look?

@hachikuji

Copy link
Copy Markdown
Contributor

LGTM

@ijuma

ijuma commented May 3, 2016

Copy link
Copy Markdown
Member

LGTM apart from a typo in the PR title, it's PartitionAssignor.

@Ishiihara Ishiihara changed the title MINOR: Clean up of ConsumerCoordinator and PartitionAssigner. MINOR: Clean up of ConsumerCoordinator and PartitionAssignor. May 3, 2016
@Ishiihara

Copy link
Copy Markdown
Contributor Author

@ijuma Thanks for the catch and careful review!

@asfgit asfgit closed this in af01378 May 3, 2016
asfgit pushed a commit that referenced this pull request May 3, 2016
Author: Liquan Pei <liquanpei@gmail.com>

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

Closes #1306 from Ishiihara/minor-consumer-cleanup

(cherry picked from commit af01378)
Signed-off-by: Ismael Juma <ismael@juma.me.uk>
@Ishiihara Ishiihara deleted the minor-consumer-cleanup branch May 3, 2016 23:45
gfodor pushed a commit to AltspaceVR/kafka that referenced this pull request Jun 3, 2016
Author: Liquan Pei <liquanpei@gmail.com>

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

Closes apache#1306 from Ishiihara/minor-consumer-cleanup
efeg added a commit to efeg/kafka that referenced this pull request May 29, 2024
mumrah pushed a commit to mumrah/kafka that referenced this pull request Aug 14, 2024
…ache#1306) (#21)

Co-authored-by: Apoorv Mittal <apoorvmittal10@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants