MINOR: Some cleanups and additional testing for KIP-88#2383
MINOR: Some cleanups and additional testing for KIP-88#2383hachikuji wants to merge 3 commits intoapache:trunkfrom
Conversation
|
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): |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM. Thanks for improving the code for KIP-88. |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the PR, it's a good improvement in terms of readability. I left a few comments, two of them affect the behaviour and are important to address.
| } | ||
|
|
||
|
|
||
| private List<Struct> getTopicArray(Map<TopicPartition, PartitionData> responseData) { |
There was a problem hiding this comment.
Looking at this method initially, I thought it should be static since we are passing responseData as a parameter instead of accessing the field, but then I noticed that we access the struct field. Is there a reason why it's done this way? Maybe we should remove the existing parameter or pass both as parameters.
Also, should the method name be topicArray?
| */ | ||
| public OffsetFetchResponse(Errors topLevelError) { | ||
| this(topLevelError, new ArrayList<TopicPartition>(), CURRENT_VERSION); | ||
| public OffsetFetchResponse(Errors error, Map<TopicPartition, PartitionData> responseData) { |
There was a problem hiding this comment.
Is it worth adding a comment to this constructor? Maybe stating the version.
| case None => | ||
| // Return offsets for all partitions owned by this consumer group. (this only applies to consumers | ||
| // that commit offsets to Kafka.) | ||
| group.allOffsets.mapValues { offsetAndMetadata => |
There was a problem hiding this comment.
mapValues returns a view over the original collection (annoyingly), which means that the returned map can change after it's returned if group.allOffsets is updated. This doesn't seem to be what we want here.
|
|
||
| assertEquals((Errors.NONE, Map.empty), groupCoordinator.handleFetchOffsets(groupId)) | ||
|
|
||
| val commitOffsetResult = commitOffsets(groupId, OffsetCommitRequest.DEFAULT_MEMBER_ID, |
There was a problem hiding this comment.
Nit: maybe we could commit at least 2 offsets to verify that the code really returns all instead of just one?
| offsetFetchRequest.getErrorResponse(error) | ||
| else { | ||
| // clients are not allowed to see offsets for topics that are not authorized for Describe | ||
| val authorizedPartitionData = allPartitionData.filterKeys(authorizeTopicDescribe) |
There was a problem hiding this comment.
filterKeys (like mapValues) returns a view over the original collection. That means that calling get on the resulting map would call authorize which is not what we want.
|
@ijuma Thanks for reviewing. Pushed another commit to address your comments. |
|
LGTM, thanks. |
Author: Jason Gustafson <jason@confluent.io> Reviewers: Vahid Hashemian <vahidhashemian@us.ibm.com>, Ismael Juma <ismael@juma.me.uk> Closes #2383 from hachikuji/minor-cleanup-kip-88
|
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): |
|
Refer to this link for build results (access rights to CI server needed): |
Author: Jason Gustafson <jason@confluent.io> Reviewers: Vahid Hashemian <vahidhashemian@us.ibm.com>, Ismael Juma <ismael@juma.me.uk> Closes apache#2383 from hachikuji/minor-cleanup-kip-88
4 participants
No description provided.