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

Scoped the default S3 endpoint to region of the client #9459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raghuvanshraj
Copy link
Contributor

@raghuvanshraj raghuvanshraj commented Aug 21, 2023

Description

The default S3 endpoint in the repository-s3 plugin was the S3 global endpoint, which caused buckets created in regions other than in us-east-1 to fail PUT repository calls with a 307 due to delay in the DNS propagation for the new bucket. This change modifies the default endpoint to the region specific one.

Related Issues

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@raghuvanshraj
Copy link
Contributor Author

macOS precommit failing due to the following issue: actions/runner-images#8104

time="2023-08-22T08:46:42Z" level=warning msg="[hostagent] failed to open the QMP socket \"/Users/runner/.lima/colima/qmp.sock\", forcibly killing QEMU"

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.classMethod
      1 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.testResizeQueueDown

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 70.42%. Comparing base (48d4087) to head (6a96cbc).
Report is 1011 commits behind head on main.

Files Patch % Lines
...org/opensearch/repositories/s3/S3AsyncService.java 50.00% 0 Missing and 2 partials ⚠️
...java/org/opensearch/repositories/s3/S3Service.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9459      +/-   ##
============================================
- Coverage     71.09%   70.42%   -0.68%     
+ Complexity    57433    56780     -653     
============================================
  Files          4776     4776              
  Lines        270811   270817       +6     
  Branches      39582    39584       +2     
============================================
- Hits         192545   190728    -1817     
- Misses        62068    63792    +1724     
- Partials      16198    16297      +99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raghuvanshraj
Copy link
Contributor Author

Tagging @dblock for review.

Comment on lines +207 to +209
String s3Endpoint = Strings.hasText(clientSettings.region)
? "s3." + Region.of(clientSettings.region).toString() + ".amazonaws.com"
: GLOBAL_S3_ENDPOINT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we retrieve the endpoint from sdk itself , else this will break for CN and other non-classic partitions .

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Good call. This needs some tests, please.

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Sep 27, 2023
@dblock
Copy link
Member

dblock commented Sep 28, 2023

@raghuvanshraj want to finish this? needs tests

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Sep 29, 2023
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Dec 18, 2023
@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Dec 27, 2023
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Jan 27, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Feb 28, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Mar 31, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants