-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Tiered Storage] Add logs for cleanup offloaded data operation #9852
[Tiered Storage] Add logs for cleanup offloaded data operation #9852
Conversation
@@ -2764,6 +2764,7 @@ private void offloadLoop(CompletableFuture<PositionImpl> promise, Queue<LedgerIn | |||
scheduledExecutor, name) | |||
.whenComplete((ignore2, exception) -> { | |||
if (exception != null) { | |||
log.error("Failed to offload data for ledgerId {}", ledgerId, exception); |
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.
It's better to also print the managed ledger name so that we can distinguish which topic occurs the problem.
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.
Ok, I'll fix.
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.
For consistency, it might be helpful to also add the ledger's name to the warning log that occurs when the cleanup offload fails (line 2962).
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.
@michaeljmarshall Ok, thanks for reminding me.
@@ -2954,6 +2955,7 @@ private void cleanupOffloaded(long ledgerId, UUID uuid, String offloadDriverName | |||
* identify offloader | |||
*/ | |||
Map<String, String> offloadDriverMetadata, String cleanupReason) { | |||
log.info("cleanup offloaded data uuid {} for reason {}", uuid.toString(), cleanupReason); |
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.
It's better to also print the managed ledger name so that we can distinguish which topic occurs the problem. And also print the ledgerId? it's useful for debugging.
Use info level log looks good here since the cleanupOffloaded only happens while offload failed right @gaoran10
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.
Yes, I think the cleanup offloaded data is a low-frequency operation.
@@ -2764,6 +2764,7 @@ private void offloadLoop(CompletableFuture<PositionImpl> promise, Queue<LedgerIn | |||
scheduledExecutor, name) | |||
.whenComplete((ignore2, exception) -> { | |||
if (exception != null) { | |||
log.error("Failed to offload data for ledgerId {}", ledgerId, exception); |
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.
For consistency, it might be helpful to also add the ledger's name to the warning log that occurs when the cleanup offload fails (line 2962).
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
### Motivation The cleanup offloaded data operation was lack of logs, it's hard for users to analyze the tiered storage data loss reason. ### Modifications Add some logs for the cleanup offloaded data operation. (cherry picked from commit 3120a4b)
Motivation
The cleanup offloaded data operation was lack of logs, it's hard for users to analyze the tiered storage data loss reason.
Modifications
Add some logs for the cleanup offloaded data operation.