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-8640: Replace OffsetFetch request with automated protocol #7062

Merged
merged 2 commits into from
Jul 30, 2019
Merged

KAFKA-8640: Replace OffsetFetch request with automated protocol #7062

merged 2 commits into from
Jul 30, 2019

Conversation

abbccdda
Copy link
Contributor

As title.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@abbccdda abbccdda force-pushed the offset_fetch branch 3 times, most recently from 354add3 to e454d50 Compare July 10, 2019 19:50
@abbccdda
Copy link
Contributor Author

retest this please

hachikuji
hachikuji reviewed Jul 12, 2019
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, left a few comments. I think we will need some more thorough test coverage, especially in light of recent regressions.

clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchResponse.java Outdated
for (OffsetFetchResponsePartition partition : topic.partitions()) {
responseData.put(new TopicPartition(topic.name(), partition.partitionIndex()),
new PartitionData(partition.committedOffset(),
Optional.of(partition.committedLeaderEpoch()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. partition.committedLeaderEpoch() is an int, so this will never be Optional.empty(). Shouldn't we be using RequestUtils.getLeaderEpoch?

clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchResponse.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java Outdated Show resolved Hide resolved
@abbccdda
Copy link
Contributor Author

@hachikuji Addressed comments

@hachikuji
Copy link
Contributor

retest this please

@abbccdda abbccdda force-pushed the offset_fetch branch 4 times, most recently from 5db9062 to 18b4854 Compare July 17, 2019 16:58
@abbccdda
Copy link
Contributor Author

Flakey test GroupEndToEndAuthorizationTest#testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl

hachikuji
hachikuji reviewed Jul 17, 2019
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, left a few more comments. I think it would be helpful to have add some round trip test cases in MessageTest as well. Specifically we can verify round trips which include the all topic case and the different metadata/leader epochs.

clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchResponse.java Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/OffsetFetchRequestTest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/OffsetFetchRequestTest.java Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/OffsetFetchResponseTest.java Outdated Show resolved Hide resolved
@abbccdda abbccdda force-pushed the offset_fetch branch 2 times, most recently from e086e21 to 6dc9c0d Compare July 19, 2019 19:10
@abbccdda
Copy link
Contributor Author

retest this please

@abbccdda
Copy link
Contributor Author

1/3 green
retest this please

@abbccdda
Copy link
Contributor Author

retest this please

@abbccdda
Copy link
Contributor Author

retest this please

hachikuji
hachikuji reviewed Jul 29, 2019
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, just a couple more comments. Would be good to do a system test run covering the following tests: tests/kafkatest/tests/core/compatibility_test_new_broker_test.py and tests/kafkatest/tests/client/client_compatibility_produce_consume_test.py.

clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchResponse.java
private static final List<Errors> PARTITION_ERRORS = Collections.singletonList(Errors.UNKNOWN_TOPIC_OR_PARTITION);

private final Map<TopicPartition, PartitionData> responseData;
public final OffsetFetchResponseData data;
private final Errors error;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this field? Could we construct it from OffsetFetchResponseData instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to store this field because we don't want to do the initialization every time if someone tries to access it. This makes the error construction one-time thing.

clients/src/test/java/org/apache/kafka/common/message/MessageTest.java Show resolved Hide resolved
@abbccdda
Copy link
Contributor Author

Mark that we already got 3/3 green builds.

@abbccdda
Copy link
Contributor Author

Jason mentioned system test builds are both passing:
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2841/
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2842/

hachikuji
hachikuji approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

@hachikuji hachikuji merged commit 2047108 into apache:trunk Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hachikuji hachikuji hachikuji approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@abbccdda @hachikuji