OpenSearch icon indicating copy to clipboard operation
OpenSearch copied to clipboard

Register `s3.client.<client>.disable_chunked_encoding` in repository-s3 node settings

Open Gautam-aman opened this issue 4 weeks ago โ€ข 15 comments

Description

This PR registers the s3.client.<client>.disable_chunked_encoding setting in the repository-s3 plugin.

Currently, configuring the setting in opensearch.yml such as:

s3.client.default.disable_chunked_encoding: true

results in a startup failure : unknown setting [s3.client.default.disable_chunked_encoding]

even though DISABLE_CHUNKED_ENCODING is defined in S3ClientSettings and supported at repository level.

Root Cause : 
DISABLE_CHUNKED_ENCODING was not included in the list returned by S3RepositoryPlugin#getSettings(), so the setting was never registered as a valid node setting.

Fix:
Added S3ClientSettings.DISABLE_CHUNKED_ENCODING to S3RepositoryPlugin#getSettings().
Added a test assertion in S3RepositoryPluginTests#testGetExecutorBuilders to ensure the setting is registered.

Testing
./gradlew :plugins:repository-s3:check passes successfully.

Impact : 
Users can now safely configure s3.client.default.disable_chunked_encoding in opensearch.yml for S3-compatible storage that does not support AWS chunked encoding (e.g., Oracle Object Storage, MinIO, Ceph, Cloudflare R2, etc.).


<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

* **New Features**
  * Exposes additional S3 client settings: request timeout, connection timeout, connection TTL, max connections, max sync connections, connection acquisition timeout, max pending connection acquires, and a chunked-encoding toggle.

* **Tests**
  * Test suite updated to assert the expanded set of S3 configuration settings.

* **Other**
  * Internal equality/hash behavior adjusted; public APIs unchanged.

<sub>โœ๏ธ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Gautam-aman avatar Dec 03 '25 18:12 Gautam-aman

Walkthrough

Exposes eight additional S3 client configuration settings via S3RepositoryPlugin, updates S3ClientSettings equality/hash/refine logic to include maxSyncConnections and primitive comparison for disableChunkedEncoding, and extends tests to assert the newly exposed settings.

Changes

Cohort / File(s) Summary
S3 Repository Plugin
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java
Both occurrences of getSettings() now include additional S3 client configuration settings added after S3ClientSettings.SIGNER_OVERRIDE: 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. Comment updated from "named s3 client configuration settings" to "s3 client configuration settings".
S3 Client Settings
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
equals() updated to compare maxSyncConnections and use primitive equality for disableChunkedEncoding; hashCode() now includes maxSyncConnections; refine(Settings) adds a check comparing newMaxSyncConnections == maxSyncConnections.
Tests
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java
testGetExecutorBuilders expanded to assert presence of DISABLE_CHUNKED_ENCODING and the new S3 client settings (REQUEST_TIMEOUT_SETTING, CONNECTION_TIMEOUT_SETTING, CONNECTION_TTL_SETTING, MAX_CONNECTIONS_SETTING, MAX_SYNC_CONNECTIONS_SETTING, CONNECTION_ACQUISITION_TIMEOUT, MAX_PENDING_CONNECTION_ACQUIRES).

Estimated code review effort

๐ŸŽฏ 2 (Simple) | โฑ๏ธ ~12 minutes

  • Check both getSettings() occurrences for consistent ordering and no duplicate entries.
  • Verify S3ClientSettings.equals() and hashCode() include maxSyncConnections consistently and that disableChunkedEncoding uses the intended primitive comparison.
  • Confirm tests assert the exact setting objects/names as exposed.

Poem

๐Ÿฐ I hopped through settings, one by one,
New timeouts, limits โ€” now there's more fun.
Max sync counts counted true and neat,
Hashes and equals now dance in beat.
๐Ÿฅ• Small hops, big clarity โ€” code complete!

Pre-merge checks and finishing touches

โŒ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage โš ๏ธ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
โœ… Passed checks (2 passed)
Check name Status Explanation
Title check โœ… Passed The title accurately describes the main change: registering the disable_chunked_encoding setting in the S3 repository plugin, which aligns with the PR's primary objective.
Description check โœ… Passed The description includes the required sections: clear explanation of the problem, root cause analysis, the fix applied, testing confirmation, and user impact. All key information is present and well-structured.
โœจ Finishing touches
  • [ ] ๐Ÿ“ Generate docstrings
๐Ÿงช Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

๐Ÿ“œ Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

๐Ÿ“ฅ Commits

Reviewing files that changed from the base of the PR and between f9ec501cb5274d96f4c7e03f8a56d55cde32d6a4 and 1da98e9450970f9b8ac7cd9929cc8b3374792008.

๐Ÿ“’ Files selected for processing (3)
  • plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (3 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 (3)
  • plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryPluginTests.java
  • plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
  • plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.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

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Dec 03 '25 18:12 coderabbitai[bot]

:x: Gradle check result for bfa670a02edb1091443eaf9ecbd7a7d8c5427a35: 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?

github-actions[bot] avatar Dec 03 '25 18:12 github-actions[bot]

:x: Gradle check result for 217b8b76dc352f4dbf584b5c9c0fec3608c5b38b: 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?

github-actions[bot] avatar Dec 03 '25 19:12 github-actions[bot]

:x: Gradle check result for 8d42f07d7146e46c3cfa90849a1a34619f02f7db: 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?

github-actions[bot] avatar Dec 04 '25 05:12 github-actions[bot]

CI didn't start due to a Jenkins trigger 403 / jq parsing error. Requesting to retrigger gradle-check manually. Thank you!

Gautam-aman avatar Dec 04 '25 05:12 Gautam-aman

:x: Gradle check result for 068a2bd4db521ac4b9f0fb1119d7f65c0bf6656f: 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?

github-actions[bot] avatar Dec 04 '25 07:12 github-actions[bot]

I ran the suite locally via ./gradlew :plugins:repository-s3:internalClusterTest and it passes successfully.

The CI failure appears to be due to the Jenkins trigger issue (403 / jq parsing).
Requesting maintainers to manually retrigger gradle-check. Thank you!

Gautam-aman avatar Dec 04 '25 07:12 Gautam-aman

Thanks for the suggestion! ๐Ÿ‘
I will extend this PR to expose all missing S3 client settings (not only disable_chunked_encoding) as recommended.

Iโ€™m working on adding:

  • 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

Will push an update shortly.

Gautam-aman avatar Dec 04 '25 14:12 Gautam-aman

:x: Gradle check result for aeceb9a63826073e965f3bbedc3f7fe1a38c75ef: 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?

github-actions[bot] avatar Dec 04 '25 15:12 github-actions[bot]

:x: Gradle check result for 12ac7d1b6883c02b262c5a58908c1e9f7570f560: 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?

github-actions[bot] avatar Dec 04 '25 18:12 github-actions[bot]

:x: Gradle check result for 07f99f13f094d1f5a3b5f81315cf5b58823cf4f1: 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?

github-actions[bot] avatar Dec 04 '25 19:12 github-actions[bot]

:x: Gradle check result for 1d6707907cc7558dac8158d49f7127213600d780: 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?

github-actions[bot] avatar Dec 04 '25 19:12 github-actions[bot]

:x: Gradle check result for 673752607e38e0c55e761bfeaada5c286516bda9: 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?

github-actions[bot] avatar Dec 05 '25 12:12 github-actions[bot]

:x: Gradle check result for 92d39feadb3352e2218f49687284263e675cf172: 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?

github-actions[bot] avatar Dec 05 '25 12:12 github-actions[bot]

All S3 plugin tests are passing locally and on CI before cleanup. The final failure occurs during Jenkins cleanup (docker prune / workspace wipe), which indicates a flaky infra failure unrelated to the code change.

Requesting a CI re-run. ๐Ÿ™

Gautam-aman avatar Dec 05 '25 12:12 Gautam-aman

:x: Gradle check result for 23d184addefd2b15627585199f86349074a822bd: 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?

github-actions[bot] avatar Dec 19 '25 06:12 github-actions[bot]

:x: Gradle check result for f9ec501cb5274d96f4c7e03f8a56d55cde32d6a4: 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?

github-actions[bot] avatar Dec 19 '25 06:12 github-actions[bot]

I have verified that ./gradlew spotlessApply produces no changes locally and formatting passes. The Spotless failure on Jenkins is due to configuration caching (Cannot fingerprint input property 'stepsInternalEquality'), which is unrelated to changes in this PR. Requesting CI retrigger / maintainer assistance.

Gautam-aman avatar Dec 19 '25 08:12 Gautam-aman

:x: Gradle check result for 1da98e9450970f9b8ac7cd9929cc8b3374792008: 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?

github-actions[bot] avatar Dec 19 '25 08:12 github-actions[bot]