You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "apoorvmittal10 (via GitHub)" <gi...@apache.org> on 2025/10/17 17:57:46 UTC
[PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
apoorvmittal10 opened a new pull request, #20725:
URL: https://github.com/apache/kafka/pull/20725
Updating params for share partition tests for readability.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #20725:
URL: https://github.com/apache/kafka/pull/20725#issuecomment-3417236673
@chia7712 Can you please re-review.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20725:
URL: https://github.com/apache/kafka/pull/20725#discussion_r2440809883
##########
core/src/test/java/kafka/server/share/ShareFetchUtilsTest.java:
##########
@@ -657,7 +657,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
private MemoryRecords createMemoryRecords(Map<Long, Integer> recordsPerOffset) {
ByteBuffer buffer = ByteBuffer.allocate(1024);
- recordsPerOffset.forEach((offset, numOfRecords) -> memoryRecordsBuilder(buffer, numOfRecords, offset).close());
+ recordsPerOffset.forEach((offset, numOfRecords) -> memoryRecordsBuilder(buffer, offset, numOfRecords).close());
Review Comment:
```
public static MemoryRecordsBuilder memoryRecordsBuilder(ByteBuffer buffer, int numOfRecords, long startOffset) {
MemoryRecordsBuilder builder = MemoryRecords.builder(buffer, Compression.NONE,
TimestampType.CREATE_TIME, startOffset, 2);
for (int i = 0; i < numOfRecords; i++) {
builder.appendWithOffset(startOffset + i, 0L, TestUtils.randomString(10).getBytes(), TestUtils.randomString(10).getBytes());
}
return builder;
}
```
ditto
##########
core/src/test/java/kafka/server/share/ShareFetchUtilsTest.java:
##########
@@ -636,7 +636,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
}
private MemoryRecords createMemoryRecords(long baseOffset, int numRecords) {
- try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(numRecords, baseOffset)) {
+ try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(baseOffset, numRecords)) {
Review Comment:
```java
public static MemoryRecordsBuilder memoryRecordsBuilder(int numOfRecords, long startOffset) {
return memoryRecordsBuilder(ByteBuffer.allocate(1024), numOfRecords, startOffset);
}
```
it seems this change is incorrect.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20725:
URL: https://github.com/apache/kafka/pull/20725
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #20725:
URL: https://github.com/apache/kafka/pull/20725#issuecomment-3421878326
> @apoorvmittal10 thanks for this patch. While this PR does not change every order, that should be fine if all tests pass. However, some comments are now incorrect
You are right that I looked at failing tests before, however I have went through all usage of these methods and checked if they are swaped. Please let me know if I missed any.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #20725:
URL: https://github.com/apache/kafka/pull/20725#discussion_r2440867081
##########
core/src/test/java/kafka/server/share/ShareFetchUtilsTest.java:
##########
@@ -636,7 +636,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
}
private MemoryRecords createMemoryRecords(long baseOffset, int numRecords) {
- try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(numRecords, baseOffset)) {
+ try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(baseOffset, numRecords)) {
Review Comment:
Seems I missed pushing the changes in file: ShareFetchTestUtils.java. There were other local changes and missed the file. Pushed now.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #20725:
URL: https://github.com/apache/kafka/pull/20725#discussion_r2440868508
##########
core/src/test/java/kafka/server/share/ShareFetchUtilsTest.java:
##########
@@ -657,7 +657,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
private MemoryRecords createMemoryRecords(Map<Long, Integer> recordsPerOffset) {
ByteBuffer buffer = ByteBuffer.allocate(1024);
- recordsPerOffset.forEach((offset, numOfRecords) -> memoryRecordsBuilder(buffer, numOfRecords, offset).close());
+ recordsPerOffset.forEach((offset, numOfRecords) -> memoryRecordsBuilder(buffer, offset, numOfRecords).close());
Review Comment:
Same, pushed the file.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Correcting the order of params in share fetch/partition tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20725:
URL: https://github.com/apache/kafka/pull/20725#discussion_r2442458170
##########
core/src/test/java/kafka/server/share/SharePartitionTest.java:
##########
@@ -7162,7 +7162,7 @@ public void testNextFetchOffsetWithMultipleConsumers() {
.withMaxInflightRecords(100)
.withState(SharePartitionState.ACTIVE)
.build();
- MemoryRecords records1 = memoryRecords(3, 0);
+ MemoryRecords records1 = memoryRecords(3);
String memberId1 = MEMBER_ID;
String memberId2 = "member-2";
Review Comment:
please take a look at line#7176
##########
core/src/test/java/kafka/server/share/SharePartitionTest.java:
##########
@@ -7562,21 +7562,21 @@ public void testAcquireWhenBatchesAreRemovedFromBetweenInSubsequentFetchData() {
try (MemoryRecordsBuilder builder = MemoryRecords.builder(buffer, Compression.NONE,
Review Comment:
line#7559 needs to be updated too
##########
core/src/test/java/kafka/server/share/SharePartitionTest.java:
##########
@@ -6796,7 +6796,7 @@ public void testCanAcquireRecordsReturnsTrue() {
assertEquals(0, sharePartition.startOffset());
Review Comment:
please check line#6769 - it has incorrect comment now
##########
core/src/test/java/kafka/server/share/SharePartitionTest.java:
##########
@@ -7208,7 +7208,7 @@ public void testReacquireSubsetWithAnotherMember() {
MemoryRecords records1 = memoryRecords(5, 5);
fetchAcquiredRecords(sharePartition, records1, 5);
- fetchAcquiredRecords(sharePartition, memoryRecords(12, 10), 12);
+ fetchAcquiredRecords(sharePartition, memoryRecords(10, 12), 12);
sharePartition.acknowledge(MEMBER_ID, List.of(
new ShareAcknowledgementBatch(5, 11, List.of((byte) 2)),
Review Comment:
please check line#7224
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org