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

enable defaultAwsCredentialChain when botoCfgPath is neglected #627

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

waziqi89
Copy link
Contributor

@waziqi89 waziqi89 commented Mar 6, 2024

RP-10149

  • Change the default botoCfgPath to be null
  • Allow default credential chain when this value is null
  • Enable global bucket access for loading the bucket region. This is needed for the latest java SDK to getBucketLocation from a different region. (learn from RP-10121)

@waziqi89 waziqi89 requested review from aprudhomme and vim345 March 6, 2024 16:54
@waziqi89
Copy link
Contributor Author

waziqi89 commented Mar 6, 2024

The YelpReviews actually is a case where credentials and bucketname are both invalid. To keep consistency instead of throwing exceptions, we need to tolerate the AmazonS3Exception

@waziqi89 waziqi89 force-pushed the u/waziqi/RP-10149 branch 2 times, most recently from 13d12e3 to 67a201c Compare March 20, 2024 15:55
@waziqi89 waziqi89 force-pushed the u/waziqi/RP-10149 branch from 00cf2fa to 643fe83 Compare March 20, 2024 17:22
@waziqi89 waziqi89 requested a review from fragosoluana March 20, 2024 17:42
Copy link
Contributor

@vim345 vim345 left a comment

Choose a reason for hiding this comment

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

LGTM. I just had a question regarding the dummy S3 client.

+ luceneServerConfiguration.getBucketName()
+ ". This could be caused by missing credentials and/or regions, or wrong bucket name.",
sdkClientException);
logger.info("return a dummy AmazonS3.");
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the trade off of using a dummy Amazon S3 client here?

Copy link
Contributor Author

@waziqi89 waziqi89 Mar 25, 2024

Choose a reason for hiding this comment

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

This is used to keep the consistency after making the changes. This is essential for some existing tests where the s3 settings is invalid.
https://github.com/Yelp/nrtsearch/pull/627/files#diff-37db6e5b57c28cce4fd4b60b04b1e1c8308668c90a4d5ec9d1ee6bac6becfe1aL53

@waziqi89 waziqi89 merged commit 915da1a into Yelp:master Mar 25, 2024
1 check passed
@waziqi89 waziqi89 deleted the u/waziqi/RP-10149 branch December 13, 2024 14:52
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