[GitHub] [kafka] ableegoldman commented on a change in pull request #10000: KAFKA-9274: handle TimeoutException on task reset
ableegoldman commented on a change in pull request #1:
URL: https://github.com/apache/kafka/pull/1#discussion_r570495244
##
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##
@@ -227,6 +230,27 @@ public void initializeIfNeeded() {
}
}
+private void initOffsetsIfNeeded(final
java.util.function.Consumer> offsetResetter) {
Review comment:
Hm...I'm not necessarily that concerned about calling
`mainConsumer.committed` twice in rare cases (although maybe that would not be
so good, since those rare cases happen to be those in which this is probably
more likely to time out, right?)
But personally, just coming into this code from the outside, it's super
confusing to have two different methods for initializing the offsets. It seems
more convoluted that way, to me. Also maybe I am missing some context here but
why do we call `initOffsetsIfNeeded` from `initializeIfNeeded` rather than
from `completeRestoration` in the first place? We don't need to initialize main
consumer offsets until it transitions to running
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10000: KAFKA-9274: handle TimeoutException on task reset
ableegoldman commented on a change in pull request #1:
URL: https://github.com/apache/kafka/pull/1#discussion_r570535029
##
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##
@@ -227,6 +230,27 @@ public void initializeIfNeeded() {
}
}
+private void initOffsetsIfNeeded(final
java.util.function.Consumer> offsetResetter) {
+final Map committed =
mainConsumer.committed(resetOffsetsForPartitions);
+for (final Map.Entry committedEntry
: committed.entrySet()) {
+final OffsetAndMetadata offsetAndMetadata =
committedEntry.getValue();
+if (offsetAndMetadata != null) {
+mainConsumer.seek(committedEntry.getKey(), offsetAndMetadata);
+resetOffsetsForPartitions.remove(committedEntry.getKey());
+}
+}
+
+if (!resetOffsetsForPartitions.isEmpty()) {
Review comment:
Can we just pass in a no-op lambda instead? I'd rather avoid special
handling for null input that isn't supposed to be null, just so we can use null
in the tests (which are therefore not realistic tests since it should never be
null, no?)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10000: KAFKA-9274: handle TimeoutException on task reset
ableegoldman commented on a change in pull request #1:
URL: https://github.com/apache/kafka/pull/1#discussion_r570495244
##
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##
@@ -227,6 +230,27 @@ public void initializeIfNeeded() {
}
}
+private void initOffsetsIfNeeded(final
java.util.function.Consumer> offsetResetter) {
Review comment:
Hm...I'm not necessarily that concerned about calling
`mainConsumer.committed` twice in rare cases (although maybe that would not be
so good, since those rare cases happen to be those in which this is probably
more likely to time out, right?)
But personally, just coming into this code from the outside, it's super
confusing to have two different methods for initializing the offsets. It seems
more convoluted that way, to me. Also maybe I am missing some context here but
why do we call `initOffsetsIfNeeded` from `initializeIfNeeded` rather than
from `completeRestoration` in the first place? We don't need to initialize main
consumer offsets until it transitions to running
##
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##
@@ -227,6 +230,27 @@ public void initializeIfNeeded() {
}
}
+private void initOffsetsIfNeeded(final
java.util.function.Consumer> offsetResetter) {
+final Map committed =
mainConsumer.committed(resetOffsetsForPartitions);
+for (final Map.Entry committedEntry
: committed.entrySet()) {
+final OffsetAndMetadata offsetAndMetadata =
committedEntry.getValue();
+if (offsetAndMetadata != null) {
+mainConsumer.seek(committedEntry.getKey(), offsetAndMetadata);
+resetOffsetsForPartitions.remove(committedEntry.getKey());
+}
+}
+
+if (!resetOffsetsForPartitions.isEmpty()) {
Review comment:
Can we just pass in a no-op lambda instead? I'd rather avoid special
handling for null input that isn't supposed to be null, just so we can use null
in the tests (which are therefore not realistic tests since it should never be
null, no?)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10000: KAFKA-9274: handle TimeoutException on task reset
ableegoldman commented on a change in pull request #1:
URL: https://github.com/apache/kafka/pull/1#discussion_r571317298
##
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##
@@ -229,17 +231,22 @@ public void initializeIfNeeded() {
}
}
+public void addPartitionsForOffsetReset(final Set
partitionsForOffsetReset) {
+mainConsumer.pause(partitionsForOffsetReset);
+resetOffsetsForPartitions.addAll(partitionsForOffsetReset);
+}
+
/**
* @throws TimeoutException if fetching committed offsets timed out
*/
@Override
-public void completeRestoration() {
+public void completeRestoration(final
java.util.function.Consumer> offsetResetter) {
switch (state()) {
case RUNNING:
return;
case RESTORING:
-initializeMetadata();
+resetOffsetsIfNeededAndInitializeMetadata(offsetResetter);
Review comment:
cool, thanks, this seems much cleaner to me
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
