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

Catching AccessDeniedException for Personalize calls #195

Closed
wants to merge 1 commit into from

Conversation

mishprs
Copy link
Contributor

@mishprs mishprs commented Aug 28, 2023

Description

Catching AccessDeniedException for Personalize calls

Issues Resolved

[List any issues this PR will resolve]

Check List

  • [*] New functionality includes testing.
    • [*] All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Comment on lines +106 to +108
if (ACCESS_DENIED_EXCEPTION_ERROR_CODE.equals(e.getErrorCode())) {
throw new InvalidInputException(INSUFFCIENT_PUT_METRIC_PERMISSION_FORMAT);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order for OpenSearch to throw a 4xx error, I believe the thrown exception needs to be something that OpenSearch will recognize as a user input problem, like IllegalArgumentException.

OpenSearch doesn't know about AWS service exception classes.

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #195 (7f70b0b) into main (eb77879) will decrease coverage by 0.10%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##               main     #195      +/-   ##
============================================
- Coverage     83.26%   83.17%   -0.10%     
  Complexity      334      334              
============================================
  Files            43       43              
  Lines          1261     1266       +5     
  Branches        154      155       +1     
============================================
+ Hits           1050     1053       +3     
- Misses          134      135       +1     
- Partials         77       78       +1     
Files Changed Coverage Δ
...ng/reranker/impl/AmazonPersonalizedRankerImpl.java 93.90% <66.66%> (-2.21%) ⬇️

@sejli
Copy link
Member

sejli commented Aug 29, 2023

Saw there's another PR created (#196) with changes made to address @msfroh's comments. Could we close out this one to avoid confusion?

@noCharger
Copy link
Collaborator

noCharger commented Aug 29, 2023

Saw there's another PR created (#196) with changes made to address @msfroh's comments. Could we close out this one to avoid confusion?

To preserve the comment history, I would prefer to close #196 and revise commits here in #195.

@mishprs mishprs closed this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants