-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Register s3.client.<client>.disable_chunked_encoding in repository-s3 node settings
#20161
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
base: main
Are you sure you want to change the base?
Register s3.client.<client>.disable_chunked_encoding in repository-s3 node settings
#20161
Conversation
WalkthroughUpdated S3 repository plugin settings list (removed a subset of older settings and added new connection-related settings), expanded unit tests to assert these settings, and adjusted S3ClientSettings equality/hashCode and refine logic to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java (1)
78-79: Keep the assertion, but avoid PR-specific comment wordingThe new assertion correctly verifies that
DISABLE_CHUNKED_ENCODINGis registered in the plugin settings. The inline comment (“New assertion for your bug fix”) is tied to this PR and doesn’t convey long-term intent; consider either removing it or replacing it with something descriptive like “ensure DISABLE_CHUNKED_ENCODING is registered as a node setting”.plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
366-367: Setting registration is correct; clean up the inline bug-fix commentIncluding
S3ClientSettings.DISABLE_CHUNKED_ENCODINGingetSettings()is the right fix to register this node setting. The comment “Fixed the bug in this line” is tied to this PR and doesn’t describe behavior; consider removing it or replacing it with a brief description of what the setting does (e.g., that it controls disabling HTTP chunked encoding for S3 clients).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java(1 hunks)
|
❌ Gradle check result for bfa670a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
bfa670a to
217b8b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
366-368: DISABLE_CHUNKED_ENCODING registration is correct; consider cleaning up the commentThe addition of
S3ClientSettings.DISABLE_CHUNKED_ENCODINGtogetSettings()is exactly what’s needed for the node setting to be registered; behavior-wise this looks good.The inline comment
// Fixed the bug in this lineis not very descriptive and will age quickly. I’d either drop it or replace it with a comment about what the setting does instead of the historical note.For example, to just remove the historical comment:
- S3ClientSettings.SIGNER_OVERRIDE, - // Fixed the bug in this line - S3ClientSettings.DISABLE_CHUNKED_ENCODING, + S3ClientSettings.SIGNER_OVERRIDE, + S3ClientSettings.DISABLE_CHUNKED_ENCODING,Or, if you want a descriptive comment:
- S3ClientSettings.SIGNER_OVERRIDE, - // Fixed the bug in this line - S3ClientSettings.DISABLE_CHUNKED_ENCODING, + S3ClientSettings.SIGNER_OVERRIDE, + // Allows disabling AWS SDK chunked encoding for S3-compatible storage + S3ClientSettings.DISABLE_CHUNKED_ENCODING,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java(1 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
|
❌ Gradle check result for 217b8b7: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
217b8b7 to
8d42f07
Compare
|
❌ Gradle check result for 8d42f07: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
… settings Signed-off-by: Aman Gautam <[email protected]>
8d42f07 to
068a2bd
Compare
|
CI didn't start due to a Jenkins trigger 403 / jq parsing error. |
|
❌ Gradle check result for 068a2bd: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
I ran the suite locally via The CI failure appears to be due to the Jenkins trigger issue (403 / jq parsing). |
|
Thanks for the suggestion! 👍 I’m working on adding:
Will push an update shortly. |
Signed-off-by: Aman Gautam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
367-367: Consider revising the comment for accuracy.The comment "required new settings" is misleading—these settings already exist in
S3ClientSettingsbut were not previously registered. Consider a more accurate comment such as "additional s3 client configuration settings" or "previously unregistered s3 client settings."Apply this diff to improve the comment:
- // required new settings + // additional s3 client configuration settings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java(2 hunks)plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java(2 hunks)plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
- plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
368-375: Verify that all settings exist in S3ClientSettings.The addition of these eight settings is essential for allowing users to configure them in
opensearch.yml. Ensure that all these setting constants (REQUEST_TIMEOUT_SETTING,CONNECTION_TIMEOUT_SETTING,CONNECTION_TTL_SETTING,MAX_CONNECTIONS_SETTING,MAX_SYNC_CONNECTIONS_SETTING,CONNECTION_ACQUISITION_TIMEOUT,MAX_PENDING_CONNECTION_ACQUIRES,DISABLE_CHUNKED_ENCODING) are properly defined inS3ClientSettingsto prevent compilation or runtime issues.
Signed-off-by: Aman Gautam <[email protected]>
|
❌ Gradle check result for aeceb9a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aman Gautam <[email protected]>
|
❌ Gradle check result for 12ac7d1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aman Gautam <[email protected]>
|
❌ Gradle check result for 07f99f1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
…istered settings Signed-off-by: Aman Gautam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (1)
S3ClientSettings(66-771)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
346-371: Unable to verify this review comment due to repository access constraints. The comment makes several specific claims that require codebase examination:
- Claims 14 settings were removed from
getSettings()(based on an "AI summary" not provided)- Claims 8 settings were promised in PR objectives but only 4 were added
- References specific setting names as removed
The provided code snippet shows only the final state of
getSettings()without a diff showing what was removed. To properly verify, I would need to examine:
- The full git diff showing removed vs. added settings
- The actual PR description/objectives
- The complete S3ClientSettings.java and S3Repository.java files
- Whether the allegedly removed settings are actually defined elsewhere in the codebase
Without access to the actual codebase, I cannot confirm:
- Whether these settings were actually removed
- Whether they are still accessible through other means
- The accuracy of the "8 promised settings" claim
- Whether this truly constitutes a breaking change for production systems
The review comment contains actionable concerns, but the factual claims require codebase verification to assess validity.
| S3ClientSettings.MAX_SYNC_CONNECTIONS_SETTING, | ||
| S3ClientSettings.CONNECTION_ACQUISITION_TIMEOUT, | ||
| S3ClientSettings.MAX_PENDING_CONNECTION_ACQUIRES, | ||
| S3ClientSettings.DISABLE_CHUNKED_ENCODING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete implementation: Missing 4 promised settings.
In the PR comments, you stated you would add 8 settings: REQUEST_TIMEOUT_SETTING, CONNECTION_TIMEOUT_SETTING, CONNECTION_TTL_SETTING, MAX_CONNECTIONS_SETTING, MAX_SYNC_CONNECTIONS_SETTING, CONNECTION_ACQUISITION_TIMEOUT, MAX_PENDING_CONNECTION_ACQUIRES, and DISABLE_CHUNKED_ENCODING.
However, only 4 settings were added here. The missing settings are:
S3ClientSettings.REQUEST_TIMEOUT_SETTINGS3ClientSettings.CONNECTION_TIMEOUT_SETTINGS3ClientSettings.CONNECTION_TTL_SETTINGS3ClientSettings.MAX_CONNECTIONS_SETTING
These are important S3 client configuration settings that should be registered for users to configure connection behavior.
Apply this diff to add the missing settings:
// additional s3 client configuration settings added in this PR
+ S3ClientSettings.REQUEST_TIMEOUT_SETTING,
+ S3ClientSettings.CONNECTION_TIMEOUT_SETTING,
+ S3ClientSettings.CONNECTION_TTL_SETTING,
+ S3ClientSettings.MAX_CONNECTIONS_SETTING,
S3ClientSettings.MAX_SYNC_CONNECTIONS_SETTING,
S3ClientSettings.CONNECTION_ACQUISITION_TIMEOUT,
S3ClientSettings.MAX_PENDING_CONNECTION_ACQUIRES,
S3ClientSettings.DISABLE_CHUNKED_ENCODING
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| S3ClientSettings.MAX_SYNC_CONNECTIONS_SETTING, | |
| S3ClientSettings.CONNECTION_ACQUISITION_TIMEOUT, | |
| S3ClientSettings.MAX_PENDING_CONNECTION_ACQUIRES, | |
| S3ClientSettings.DISABLE_CHUNKED_ENCODING | |
| // additional s3 client configuration settings added in this PR | |
| S3ClientSettings.REQUEST_TIMEOUT_SETTING, | |
| S3ClientSettings.CONNECTION_TIMEOUT_SETTING, | |
| S3ClientSettings.CONNECTION_TTL_SETTING, | |
| S3ClientSettings.MAX_CONNECTIONS_SETTING, | |
| S3ClientSettings.MAX_SYNC_CONNECTIONS_SETTING, | |
| S3ClientSettings.CONNECTION_ACQUISITION_TIMEOUT, | |
| S3ClientSettings.MAX_PENDING_CONNECTION_ACQUIRES, | |
| S3ClientSettings.DISABLE_CHUNKED_ENCODING | |
| ); |
🤖 Prompt for AI Agents
In
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java
around lines 366 to 369, the settings list only includes
MAX_SYNC_CONNECTIONS_SETTING, CONNECTION_ACQUISITION_TIMEOUT,
MAX_PENDING_CONNECTION_ACQUIRES and DISABLE_CHUNKED_ENCODING but omitted the
four promised settings; update the settings registration to also include
S3ClientSettings.REQUEST_TIMEOUT_SETTING,
S3ClientSettings.CONNECTION_TIMEOUT_SETTING,
S3ClientSettings.CONNECTION_TTL_SETTING and
S3ClientSettings.MAX_CONNECTIONS_SETTING so all eight configured S3 client
settings are registered and available for user configuration.
|
❌ Gradle check result for 1d67079: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
This PR registers the
s3.client.<client>.disable_chunked_encodingsetting in therepository-s3plugin.Currently, configuring the setting in
opensearch.ymlsuch as: