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

Add s3 sink client options #4959

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Conversation

oeyh
Copy link
Collaborator

@oeyh oeyh commented Sep 19, 2024

Description

Adds max_connections and acquire_timeout options to s3 sink client.

Example:

...
sink:
  - s3:
      client:
          max_connections: 100
          acquire_timeout: 10s
...

Issues Resolved

Resolves #4949

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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 +15 to +16
private static final int DEFAULT_MAX_CONNECTIONS = 50;
private static final Duration DEFAULT_ACQUIRE_TIMEOUT = Duration.ofSeconds(10);
Copy link
Member

Choose a reason for hiding this comment

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

Are these the default values for SdkAsyncHttpClient ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes they are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be new defaults for every new S3 sink pipeline?

Copy link
Collaborator Author

@oeyh oeyh Sep 19, 2024

Choose a reason for hiding this comment

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

Good question. I didn't change how the S3 async client is built when client options are not provided. So the pipeline should remain the same before and after this change if client options are not explicitly configured.

dinujoh
dinujoh previously approved these changes Sep 19, 2024
dlvenable
dlvenable previously approved these changes Sep 19, 2024
private static final Duration DEFAULT_ACQUIRE_TIMEOUT = Duration.ofSeconds(10);

@JsonProperty("max_connections")
@Min(1)
Copy link
Member

Choose a reason for hiding this comment

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

Can we give a sanity upper bound? Maybe 5000?

@oeyh oeyh merged commit aaef847 into opensearch-project:main Sep 20, 2024
45 of 47 checks passed
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.

Make max connections and acquire timeout configurable on S3 sink client
4 participants