MINOR: Correcting the order of params in share fetch/partition tests #20725
Conversation
github-actions
bot
added
triage
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@apoorvmittal10 thanks for this patch
|
|
||
| private MemoryRecords createMemoryRecords(long baseOffset, int numRecords) { | ||
| try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(numRecords, baseOffset)) { | ||
| try (MemoryRecordsBuilder recordsBuilder = memoryRecordsBuilder(baseOffset, numRecords)) { |
There was a problem hiding this comment.
public static MemoryRecordsBuilder memoryRecordsBuilder(int numOfRecords, long startOffset) {
return memoryRecordsBuilder(ByteBuffer.allocate(1024), numOfRecords, startOffset);
}it seems this change is incorrect.
There was a problem hiding this comment.
Seems I missed pushing the changes in file: ShareFetchTestUtils.java. There were other local changes and missed the file. Pushed now.
| 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()); |
There was a problem hiding this 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
There was a problem hiding this comment.
Same, pushed the file.
|
@chia7712 Can you please re-review. |
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@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
| memoryRecords(2, 1).records().forEach(builder::append); | ||
| } | ||
| // Do not include batch from offset 5. And compact batch starting at offset 20. | ||
| try (MemoryRecordsBuilder builder = MemoryRecords.builder(buffer, Compression.NONE, |
There was a problem hiding this comment.
line#7559 needs to be updated too
| fetchAcquiredRecords(sharePartition, memoryRecords(10, 12), 12); | ||
|
|
||
| sharePartition.acknowledge(MEMBER_ID, List.of( | ||
| new ShareAcknowledgementBatch(5, 11, List.of((byte) 2)), |
| MemoryRecords records1 = memoryRecords(3); | ||
| String memberId1 = MEMBER_ID; | ||
| String memberId2 = "member-2"; | ||
|
|
There was a problem hiding this comment.
please take a look at line#7176
| public void testCanAcquireRecordsReturnsTrue() { | ||
| SharePartition sharePartition = SharePartitionBuilder.builder().withState(SharePartitionState.ACTIVE).build(); | ||
|
|
||
| assertEquals(0, sharePartition.startOffset()); |
There was a problem hiding this comment.
please check line#6769 - it has incorrect comment now
You are right that I looked at failing tests before, however I have went through all usage of these methods now and checked if they are swaped. Please let me know if I missed any. |
…pache#20725) Updating params for share partition tests for readability. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
2 participants
Updating params for share partition tests for readability.
Reviewers: Chia-Ping Tsai chia7712@gmail.com