mjsax commented on code in PR #13300:
URL: https://github.com/apache/kafka/pull/13300#discussion_r1123947731
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -986,6 +987,11 @@ private void prepareChangelogs(final
Set<ChangelogMetadata> newPartitionsToResto
} catch (final Exception e) {
throw new StreamsException("State restore listener failed
on batch restored", e);
}
+
+ final TaskId taskId =
changelogs.get(partition).stateManager.taskId();
+ final StreamTask task = (StreamTask) tasks.get(taskId);
+ final long recordsToRestore =
Math.max(changelogMetadata.restoreEndOffset - startOffset, 0L);
Review Comment:
Why do we need to apply `max()` ?
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -457,15 +457,9 @@ public long restore(final Map<TaskId, Task> tasks) {
// small batches; this can be optimized in the future,
e.g. wait longer for larger batches.
final TaskId taskId =
changelogs.get(partition).stateManager.taskId();
try {
+ final Task task = tasks.get(taskId);
final ChangelogMetadata changelogMetadata =
changelogs.get(partition);
- final int restored = restoreChangelog(changelogMetadata);
- if (restored > 0 ||
changelogMetadata.state().equals(ChangelogState.COMPLETED)) {
- final Task task = tasks.get(taskId);
- if (task != null) {
Review Comment:
Why do we remove this `null` check?
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -986,6 +987,11 @@ private void prepareChangelogs(final
Set<ChangelogMetadata> newPartitionsToResto
} catch (final Exception e) {
throw new StreamsException("State restore listener failed
on batch restored", e);
}
+
+ final TaskId taskId =
changelogs.get(partition).stateManager.taskId();
+ final StreamTask task = (StreamTask) tasks.get(taskId);
+ final long recordsToRestore =
Math.max(changelogMetadata.restoreEndOffset - startOffset, 0L);
+ task.recordRestoreRemaining(time, recordsToRestore);
Review Comment:
Would it be helpful to rename this method to `initRestoreRemaining` to make
it's purpuse clear (and/or add a JavaDoc to the method in `StreamTask`)?
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/TaskMetrics.java:
##########
@@ -52,6 +55,21 @@ private TaskMetrics() {}
private static final String PUNCTUATE_AVG_LATENCY_DESCRIPTION =
AVG_LATENCY_DESCRIPTION + PUNCTUATE_DESCRIPTION;
private static final String PUNCTUATE_MAX_LATENCY_DESCRIPTION =
MAX_LATENCY_DESCRIPTION + PUNCTUATE_DESCRIPTION;
+ private static final String RESTORE = "restore";
+ private static final String RESTORE_DESCRIPTION = "records restored";
+ private static final String RESTORE_TOTAL_DESCRIPTION = TOTAL_DESCRIPTION
+ RESTORE_DESCRIPTION;
+ private static final String RESTORE_RATE_DESCRIPTION =
+ RATE_DESCRIPTION_PREFIX + RESTORE_DESCRIPTION +
RATE_DESCRIPTION_SUFFIX;
+
+ private static final String UPDATE = "update";
Review Comment:
`update` or `updated`?
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/TaskMetrics.java:
##########
@@ -52,6 +55,21 @@ private TaskMetrics() {}
private static final String PUNCTUATE_AVG_LATENCY_DESCRIPTION =
AVG_LATENCY_DESCRIPTION + PUNCTUATE_DESCRIPTION;
private static final String PUNCTUATE_MAX_LATENCY_DESCRIPTION =
MAX_LATENCY_DESCRIPTION + PUNCTUATE_DESCRIPTION;
+ private static final String RESTORE = "restore";
Review Comment:
`restore` or `restored` ?
--
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
