-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR; Validate at least one control record #15912
Changes from 2 commits
323fd14
b020963
f3892c5
c98cfb6
f40a176
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,7 +256,7 @@ public void appendControlMessages(MemoryRecordsCreator valueCreator) { | |
} | ||
|
||
private int validateMemoryRecordAndReturnCount(MemoryRecords memoryRecord) { | ||
// Confirm that it is at most one batch and it is a control record | ||
// Confirm that it is one control batch and it is at least one control record | ||
Iterator<MutableRecordBatch> batches = memoryRecord.batches().iterator(); | ||
if (!batches.hasNext()) { | ||
throw new IllegalArgumentException("valueCreator didn't create a batch"); | ||
|
@@ -284,6 +284,8 @@ private int validateMemoryRecordAndReturnCount(MemoryRecords memoryRecord) { | |
); | ||
} else if (numberOfRecords == null) { | ||
throw new IllegalArgumentException("valueCreator didn't create a batch with the count"); | ||
} else if (numberOfRecords < 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to add new UT to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the test. |
||
throw new IllegalArgumentException("valueCreator didn't create at least one control record"); | ||
} else if (batches.hasNext()) { | ||
throw new IllegalArgumentException("valueCreator created more than one batch"); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ | |
* | ||
* It encapsulates static information like a voter's endpoint and their supported kraft.version. | ||
* | ||
* It providees functionality for converting to and from {@code VotersRecord} and for converting | ||
* It provides functionality for converting to and from {@code VotersRecord} and for converting | ||
* from the static configuration. | ||
*/ | ||
final public class VoterSet { | ||
|
@@ -161,8 +161,8 @@ public VotersRecord toVotersRecord(short version) { | |
* An overlapping majority means that for all majorities in {@code this} set of voters and for | ||
* all majority in {@code that} set of voters, they have at least one voter in common. | ||
* | ||
* If this function returns true is means that one of the voter set commits an offset, it means | ||
* that the other voter set cannot commit a conflicting offset. | ||
* If this function returns true, it means that if one of the set of voters commits an offset, | ||
* the other set of voters cannot commit a conflicting offset. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the the other => the other |
||
* | ||
* @param that the other voter set to compare | ||
* @return true if they have an overlapping majority, false otherwise | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateMemoryRecordAndReturnCount => validateMemoryRecordsAndReturnCount
memoryRecord => memoryRecords
Also, there is an existing typo creatte on line 268.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.