-
Notifications
You must be signed in to change notification settings - Fork 14.5k
KAFKA-19530 RemoteLogManager should record lag stats when remote storage is offline #20218
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
Conversation
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.
LGTM
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java
Outdated
Show resolved
Hide resolved
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java
Outdated
Show resolved
Hide resolved
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.
Thanks for the PR! Left one comment. PTAL.
@@ -973,6 +973,9 @@ public void copyLogSegmentsToRemote(UnifiedLog log) throws InterruptedException | |||
segmentIdsBeingCopied.add(segmentId); | |||
try { | |||
copyLogSegment(log, candidateLogSegment.logSegment, segmentId, candidateLogSegment.nextSegmentOffset); | |||
} catch (RemoteStorageException e) { |
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.
can we catch all exception in the catch
block and then throw it?
RemoteStorageException -> Exception
since copyLogSegment
throws InterruptedException, ExecutionException, RemoteStorageException, IOException, and CustomMetadataSizeLimitExceededException:
catch (Exception e) {
recordLagStats(log);
throw e;
}
@m1a2st |
@kamalcph, Sorry for the late reply! I was traveling last week, but I'll go through your comments ASAP. |
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.
LGTM! Thanks for the patch!
When remote storage is offline, then the segmentLag and bytesLag metrics
are not recorded. These metrics are useful to know the pending data to
upload when remote storage is down.
Reviewers: TaiJuWu [email protected], Kamal Chandraprakash
[email protected]