Re: [PR] MINOR; Validate at least one control record [kafka]
jsancio merged PR #15912: URL: https://github.com/apache/kafka/pull/15912 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
jsancio commented on PR #15912: URL: https://github.com/apache/kafka/pull/15912#issuecomment-2110323737 Merging. The errors are unrelated to this change. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
jsancio commented on code in PR #15912:
URL: https://github.com/apache/kafka/pull/15912#discussion_r1596977565
##
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java:
##
@@ -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
Review Comment:
Fixed.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
junrao commented on code in PR #15912:
URL: https://github.com/apache/kafka/pull/15912#discussion_r1596962906
##
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java:
##
@@ -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
Review Comment:
validateMemoryRecordAndReturnCount => validateMemoryRecordsAndReturnCount
memoryRecord => memoryRecords
Also, there is an existing typo creatte on line 268.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
jsancio commented on PR #15912: URL: https://github.com/apache/kafka/pull/15912#issuecomment-2104840600 @junrao @chia7712 thanks for the reviews. PR is ready for another round. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
jsancio commented on code in PR #15912:
URL: https://github.com/apache/kafka/pull/15912#discussion_r1596923121
##
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java:
##
@@ -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) {
Review Comment:
Added the test.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
chia7712 commented on code in PR #15912:
URL: https://github.com/apache/kafka/pull/15912#discussion_r1596192617
##
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java:
##
@@ -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) {
Review Comment:
Is it possible to add new UT to `BatchAccumulatorTest` for this case?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR; Validate at least one control record [kafka]
junrao commented on code in PR #15912:
URL: https://github.com/apache/kafka/pull/15912#discussion_r1595930091
##
raft/src/main/java/org/apache/kafka/raft/internals/VoterSet.java:
##
@@ -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
+ * the other set of voters cannot commit a conflicting offset.
Review Comment:
the the other => the other
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] MINOR; Validate at least one control record [kafka]
jsancio opened a new pull request, #15912: URL: https://github.com/apache/kafka/pull/15912 Validate that a control batch in the batch accumulator has at least one control record. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
