Re: [PR] KAFKA-17299: add unit tests for previous fix [kafka]
mjsax commented on PR #17919: URL: https://github.com/apache/kafka/pull/17919#issuecomment-2499028208 Merged to `trunk` and cherry-picked to `3.9`, `3.8`, and `3.7` branches. -- 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] KAFKA-17299: add unit tests for previous fix [kafka]
mjsax merged PR #17919: URL: https://github.com/apache/kafka/pull/17919 -- 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] KAFKA-17299: add unit tests for previous fix [kafka]
mjsax commented on code in PR #17919:
URL: https://github.com/apache/kafka/pull/17919#discussion_r1857256843
##
streams/src/main/java/org/apache/kafka/streams/processor/internals/PartitionGroup.java:
##
@@ -250,10 +250,11 @@ StampedRecord nextRecord(final RecordInfo info, final
long wallClockTime) {
if (queue != null) {
// get the first record from this queue.
+final int oldSize = queue.size();
record = queue.poll(wallClockTime);
if (record != null) {
---totalBuffered;
+totalBuffered -= oldSize - queue.size();
Review Comment:
> I think I get it now, under the covers poll may skip records due to
deserialization errors so we need to account for this in the totalBuffered
variable.
Exactly. -- Either deserialization errors, or invalid (negative) timestamp
extraction.
--
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] KAFKA-17299: add unit tests for previous fix [kafka]
bbejeck commented on code in PR #17919:
URL: https://github.com/apache/kafka/pull/17919#discussion_r1857225839
##
streams/src/main/java/org/apache/kafka/streams/processor/internals/PartitionGroup.java:
##
@@ -250,10 +250,11 @@ StampedRecord nextRecord(final RecordInfo info, final
long wallClockTime) {
if (queue != null) {
// get the first record from this queue.
+final int oldSize = queue.size();
record = queue.poll(wallClockTime);
if (record != null) {
---totalBuffered;
+totalBuffered -= oldSize - queue.size();
Review Comment:
I'm a little confused - since we only subtract if `poll` returns a valid
record, doesn't this always result in subtracting one from `totalBuffered`?
I think I get it now, under the covers `poll` may skip records due to
deserialization errors so we need to account for this in the `totalBuffered`
variable.
--
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] KAFKA-17299: add unit tests for previous fix [kafka]
mjsax commented on code in PR #17919:
URL: https://github.com/apache/kafka/pull/17919#discussion_r1855018774
##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java:
##
@@ -157,8 +157,8 @@ public class StreamTaskTest {
private final LogContext logContext = new LogContext("[test] ");
private final String topic1 = "topic1";
private final String topic2 = "topic2";
-private final TopicPartition partition1 = new TopicPartition(topic1, 1);
-private final TopicPartition partition2 = new TopicPartition(topic2, 1);
+private final TopicPartition partition1 = new TopicPartition(topic1, 0);
+private final TopicPartition partition2 = new TopicPartition(topic2, 0);
Review Comment:
Side cleanup -- not sure why we use partition number 1, but not 0?
--
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] KAFKA-17299: add unit tests for previous fix [kafka]
mjsax commented on code in PR #17919:
URL: https://github.com/apache/kafka/pull/17919#discussion_r1855018415
##
streams/src/main/java/org/apache/kafka/streams/processor/internals/PartitionGroup.java:
##
@@ -250,10 +250,11 @@ StampedRecord nextRecord(final RecordInfo info, final
long wallClockTime) {
if (queue != null) {
// get the first record from this queue.
+final int oldSize = queue.size();
record = queue.poll(wallClockTime);
if (record != null) {
---totalBuffered;
+totalBuffered -= (oldSize - queue.size());
Review Comment:
This is a follow up fix: when we skip more than one corrupted record, we
don't update the buffer size correctly.
--
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
