apoorvmittal10 commented on code in PR #17870:
URL: https://github.com/apache/kafka/pull/17870#discussion_r1912538632
##########
core/src/test/java/kafka/server/share/DelayedShareFetchTest.java:
##########
@@ -559,13 +561,18 @@ public void testCombineLogReadResponse() {
.withSharePartitions(sharePartitions)
.build();
- LinkedHashMap<TopicIdPartition, FetchRequest.PartitionData>
topicPartitionData = new LinkedHashMap<>();
- topicPartitionData.put(tp0, mock(FetchRequest.PartitionData.class));
- topicPartitionData.put(tp1, mock(FetchRequest.PartitionData.class));
+ LinkedHashMap<TopicIdPartition, Long> topicPartitionData = new
LinkedHashMap<>();
+ topicPartitionData.put(tp0, 0L);
+ topicPartitionData.put(tp1, 0L);
// Case 1 - logReadResponse contains tp0.
LinkedHashMap<TopicIdPartition, LogReadResult> logReadResponse = new
LinkedHashMap<>();
- logReadResponse.put(tp0, mock(LogReadResult.class));
+ LogReadResult logReadResult = mock(LogReadResult.class);
+ Records records = mock(Records.class);
+ when(records.sizeInBytes()).thenReturn(2);
+ FetchDataInfo fetchDataInfo = new
FetchDataInfo(mock(LogOffsetMetadata.class), records);
+ when(logReadResult.info()).thenReturn(fetchDataInfo);
+ logReadResponse.put(tp0, logReadResult);
Review Comment:
> since I have already created this test
[testCheckValidArguments](https://github.com/apache/kafka/pull/17870/files#diff-eef861ca89cf8d6840cf0f84919915c4c18da2e327e6587552c42f7a867d83a9R45),
I don't think we need it separately in DelayedShareFetch. I think it will be
redundant.
Hmmm, I am not sure how a `PartitionMaxBytesStrategyTest` can validate the
correct handling of exception thrown by `PartitionMaxBytesStrategy.maxBytes` in
DelayedShareFetch? i.e. The unit tests in PartitionMaxBytesStrategyTest doesn't
validate whether the handling of thrown exception is correct in
DelayedShareFetch.
> Yes, we have a test
[testShareFetchRequestSuccessfulSharingBetweenMultipleConsumers](https://github.com/apache/kafka/pull/17870/files#diff-784cd94373b734f49edc71a0b70d4f2d6d11dbf8b345746db7340b9a5f4fedbdR1386)
Why can't we have unit tests for the maxBytes in DelayedShareFetch? I think
we should have some i.e. for same share fetch bytes and partition data, the
output varies when acquired records are different. Meaning the bytes for
partitions are in accordance with partition max bytes strategy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
