Skip to content

Conversation

@arnulfojr
Copy link

@arnulfojr arnulfojr commented Jun 10, 2020

Description of changes:

At the moment, when the handler fails to schedule a re-invocation the error message is not displayed in any manner and is hard to guess what was the error of the stack when the handlers succeed on "handling" the resource operation.

This at least will make it easier for the operator to know the details of the error.

Not a required change, just making life easier in the meantime

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 574 to 576
} catch (final Throwable e) {
log(String.format("Failed to schedule re-invoke, %s", e.getMessage()));
handlerResponse.setMessage("Failed to schedule re-invoke.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch (final Throwable e) {
log(String.format("Failed to schedule re-invoke, %s", e.getMessage()));
handlerResponse.setMessage("Failed to schedule re-invoke.");
} catch (final Throwable e) {
handlerResponse.setMessage("Failed to schedule re-invoke: " + e.getMessage());

Copy link
Author

@arnulfojr arnulfojr Jun 10, 2020

Choose a reason for hiding this comment

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

Hey, thanks for the review!

I see that this was previously removed. It might not also be a good idea to add it back again. Makes sense to "not" leak info back to the customer as this is internal to the service. I just thought of adding it to the log so the operator of the resource can at least know what's failing.

52114e3#diff-dfc0e730cbc8e8034297955f8255ceb7

Is there something in the throwable that can be a potential leak if we add it to the log?
I'm not very familiar with Java's serialization to string.

@miparnisari miparnisari requested a review from rjlohan June 10, 2020 20:09
@rjlohan
Copy link
Contributor

rjlohan commented Jun 19, 2020

This change might be made redundant by #279 as we have changed the model for re-invocation to allow us to increase some limits (which may have been the very things causing the re-invoke to fail for you).

@arnulfojr arnulfojr closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants