KAFKA-14589 ConsumerGroupServiceTest rewritten in java#15248
Conversation
Co-authored-by: Taras Ledkov <tledkov@apache.org>
…merGroupCommandOptions.java Co-authored-by: Taras Ledkov <tledkov@apache.org>
|
I prepared second PR for KAFKA-14589. |
|
@tledkov can you, please, take a look? |
|
CI are OK. |
|
Hello @showuon Can you, please, take a look? |
| when(admin.listOffsets(offsetsArgMatcher(), any())) | ||
| .thenReturn(listOffsetsResult()); | ||
|
|
||
| Tuple2<Option<String>, Option<Seq<ConsumerGroupCommand.PartitionAssignmentState>>> res = groupService.collectGroupOffsets(GROUP); |
There was a problem hiding this comment.
is there a way we could say what the tuple contains? like stateAndAssignments
| TopicPartition testTopicPartition4 = new TopicPartition("testTopic2", 1); | ||
| TopicPartition testTopicPartition5 = new TopicPartition("testTopic2", 2); | ||
|
|
||
| // Some topic's partitions gets valid OffsetAndMetadata values, other gets nulls values (negative integers) and others aren't defined |
There was a problem hiding this comment.
Is this comment correct? Or the test? I don't see the negative values
There was a problem hiding this comment.
Actually, I just copied it from the scala version of test.
Removed mention about negative values.
There was a problem hiding this comment.
The test is called testAdminRequestsForDescribeNegativeOffsets so we might want to figure out if that's the original intention.
There was a problem hiding this comment.
Looked in git log and this comment here from the moment test added - so I think we want to keep it as is, because we just move them to java - d95c191#diff-fcf6c81f0d8445b938d80a9d1f5ef838f731807d27b3f22a2cfefe2762b3001eR78
There was a problem hiding this comment.
Oh I see -- at this point the negative value is already converted to null. The comment in this test was confusing.
| private ListConsumerGroupOffsetsResult listGroupOffsetsResult(String groupId) { | ||
| Map<TopicPartition, OffsetAndMetadata> offsets = TOPIC_PARTITIONS.stream().collect(Collectors.toMap( | ||
| Function.identity(), | ||
| tp -> new OffsetAndMetadata(100))); |
There was a problem hiding this comment.
we don't actually need the tp here right? Just a name for the argument we don't use. Ditto to some of the usages below.
There was a problem hiding this comment.
I think there are a few places in code we use __ to signify an unused argument. Note there are two underscores and not one like in scala.
|
Hello @jolshan |
|
Sorry I meant to check this one yesterday but accidentally looked at your other PR. I thought I was waiting on build issues there, but should have been looking here. Looking now. |
This PR is part of apache#14471 Is contains single test rewritten in java. Intention of separate PR is to reduce changes and simplify review. Reviewers: Justine Olshan <jolshan@confluent.io>
This PR is part of apache#14471 Is contains single test rewritten in java. Intention of separate PR is to reduce changes and simplify review. Reviewers: Justine Olshan <jolshan@confluent.io>
This PR is part of apache#14471 Is contains single test rewritten in java. Intention of separate PR is to reduce changes and simplify review. Reviewers: Justine Olshan <jolshan@confluent.io>
This PR is part of #14471
Is contains single test rewritten in java.
Intention of separate PR is to reduce changes and simplify review.
Committer Checklist (excluded from commit message)