Skip to content
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

Add logging to avoid loss of information when exception changes during retries #325

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

joker1007
Copy link
Contributor

If a raised exception changes during retry executions and the exception is not retryable, the retrying is aborted instantly, but the information of the last exception is lost, because RetryExecutor throws only the first exception and OnRetry hook doesn't log about the last exception.

For example, like this:

2023-09-06 18:56:18.365 +0000 [WARN] (0018:task-0010): Operation fai
led (1213:40001), retrying 1/8 after 1 seconds. Message: Deadlock found when trying to get lock; try restarting transaction
2023-09-06 18:56:19.366 +0000 [ERROR] (0018:task-0010): Operation fa
iled (1213:40001)

In this situation, it is difficult to identify the root problem, so I would like to change to log also last exception.

@dmikurube dmikurube requested a review from a team October 3, 2023 07:11
@dmikurube dmikurube added this to the v0.10.3 milestone Oct 3, 2023
Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! and sorry to be late. It's better at least than before, then approving it now.

(I was thinking about starting to use JsonValue in these plugins, which means that it wouldn't work with Embulk v0.9. However, it seems better to release this fix before JsonValue.)


Let me leave a comment and a question although approving it.

  • We may want to have the logging message formats aligned with onRetry's. Maybe nice to update also onRetry.
  • In onRetry's logging, did you have any intention for buildExceptionMessage, not just with logger.warn(..., Throwable)? (Confirming it here as you seem the author of Implements retryable execution feature #113.)

@joker1007
Copy link
Contributor Author

joker1007 commented Oct 3, 2023

A reason why I didn't use buildExceptionMessage is that the method displays only error messages.
I wanted the stack trace of the exception.

I don't think the logging format needs to be aligned with onRetry.
Because users usually don't need the stack trace of retriable exceptions but typically want the stack trace of not retriable exceptions or given up exceptions.

@dmikurube
Copy link
Member

Got it. Sounds reasonable! Let me merge this.

@dmikurube dmikurube merged commit 952cae5 into embulk:master Oct 4, 2023
@joker1007 joker1007 deleted the log-last-exception branch October 4, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants