Skip to content

feat: recreate sdk client with customized exception#1709

Draft
yitingb wants to merge 1 commit intomainfrom
retry-exception
Draft

feat: recreate sdk client with customized exception#1709
yitingb wants to merge 1 commit intomainfrom
retry-exception

Conversation

@yitingb
Copy link
Member

@yitingb yitingb commented Apr 17, 2025

Issue #, if available:

Description of changes:

Why is this change necessary:

How was this change tested:

  • Updated or added new unit tests.
  • Updated or added new integration tests.
  • Updated or added new end-to-end tests.
  • If my code makes a remote network call, it was tested with a proxy.

Any additional information or context required to review the change:

Documentation Checklist:

  • Updated the README if applicable.

Compatibility Checklist:

  • I confirm that the change is backwards compatible.
  • Any modification or deletion of public interfaces does not impact other plugin components.
  • For external library version updates, I have reviewed its change logs and Nucleus does not consume
    any deprecated method or type.

Refer to Compatibility Guidelines for more information.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

private String getAccountId() {
return stsClient.getCallerIdentity(GetCallerIdentityRequest.builder().build()).account();
return stsWrapper.execute(client ->
client.getCallerIdentity(GetCallerIdentityRequest.builder().build()).account());
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

software.amazon.awssdk.services.sts.StsClient.getCallerIdentity API can also throw the following exception types: SdkClientException, StsException, SdkException, AwsServiceException, UnsupportedOperationException. We recommend handling these uncaught exceptions as well.

private <R> R executeOperation(final Function<T, R> operation) {
try {
return operation.apply(client);
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that your code handles a broad swath of exceptions in the catch block, potentially trapping dissimilar issues or problems that should not be dealt with at this point in the program.

Learn more

public <R> R execute(final Function<T, R> operation) {
try {
return executeOperation(operation);
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that your code handles a broad swath of exceptions in the catch block, potentially trapping dissimilar issues or problems that should not be dealt with at this point in the program.

Learn more

if (client != null) {
try {
client.close();
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that your code handles a broad swath of exceptions in the catch block, potentially trapping dissimilar issues or problems that should not be dealt with at this point in the program.

Learn more

return executeOperation(operation);
} catch (Exception e) {
if (shouldRefreshClient(e)) {
System.out.println("Client needs refresh due to: " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that you are using println() rather than a dedicated logging facility makes it difficult to monitor the program behavior. We recommend to use a Java logging facility rather than System.out or System.err.

Learn more

try {
client.close();
} catch (Exception e) {
System.err.println("Error closing client: " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that you are using println() rather than a dedicated logging facility makes it difficult to monitor the program behavior. We recommend to use a Java logging facility rather than System.out or System.err.

Learn more

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.

2 participants