Skip to content

[Store] Structure S3ClientConfig environment settings - #4098

Merged
Aionw merged 2 commits into
kvcache-ai:mainfrom
bitborne:codex/s3-client-environ-convergence
Sep 17, 2026
Merged

Aionw merged 2 commits into
kvcache-ai:mainfrom
bitborne:codex/s3-client-environ-convergence

Conversation

@bitborne

@bitborne bitborne commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Description

Converges Mooncake Store's existing S3 client environment settings under a private S3ClientConfig as part of #3809.

  • Groups the eleven existing MOONCAKE_AWS_* variables in the common environment catalog using their semantic string, boolean, and int64_t types.
  • Loads the values through typed Environ::ReadOr() and caches the resolved config once from S3Helper::InitAPI() for the process lifetime.
  • Removes the S3-specific getters, backing fields, constructor reads, and singleton-coupled test from the global Environ object.
  • Preserves all existing variable names, defaults, literal string behavior, shared boolean/integer parsing, invalid-value warnings, explicit endpoint/bucket/region argument precedence, credential redaction, and restart/reinitialization caching behavior.
  • Keeps checksum-mode parsing beside the AWS SDK types. An unset or explicitly empty Mooncake checksum setting still leaves the AWS SDK default unchanged, preserving the compatibility correction reviewed in [Store] Externalize S3 client config via environment variables #2649.
  • Adds focused config tests for defaults, all populated settings, explicitly empty strings, invalid typed-value fallbacks and diagnostics, environment restoration, and independent loader reads.

Open PR #1538 is a broad cross-module migration toward a larger global Environ singleton. This PR is the narrow Store-only owner migration required by the current #3809 rules: it removes S3-specific global state, keeps the config header private, and does not change Transfer Engine or other component settings.

Open PR #2205 proposes a new checksum compatibility switch and still targets the pre-#3587 S3 helper path. This PR does not introduce that variable or adopt its behavior; if #2205 is revived, its behavior can be layered onto S3ClientConfig separately.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake Conductor (mooncake-conductor)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

cmake -S . -B build -DBUILD_UNIT_TESTS=ON -DSTORE_USE_REDIS=ON -DWITH_STORE_RUST=OFF -DBUILD_EXAMPLES=OFF -DBUILD_BENCHMARK=OFF
cmake --build build --target s3_client_config_test mooncake_master -j16
cmake --build build --target local_file_snapshot_config_test local_file_snapshot_object_store_test -j16
ctest --test-dir build --output-on-failure -R '^(environ_test|s3_client_config_test|local_file_snapshot_config_test|local_file_snapshot_object_store_test)$'
pre-commit run --files mooncake-common/include/environ.h mooncake-common/include/environment_variables.h mooncake-common/src/environ.cpp mooncake-common/tests/environ_test.cpp mooncake-store/src/CMakeLists.txt mooncake-store/src/common/s3_helper.cpp mooncake-store/src/config/s3_client_config.cpp mooncake-store/src/config/s3_client_config.h mooncake-store/tests/CMakeLists.txt mooncake-store/tests/s3_client_config_test.cpp
./scripts/code_format.sh --check --changed-lines --base kvcache/main

The Store master executable and focused config test build successfully. All four selected tests pass, touched-file pre-commit passes with pre-commit 3.7.1, and the post-commit changed-lines check passes with clang-format 20.1.8.

The local environment does not have the AWS SDK installed, so the conditionally compiled s3_helper.cpp path was not compiled locally. The SDK-independent S3ClientConfig is compiled into mooncake_master; AWS-enabled CI remains responsible for compiling the consumer path.

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

No documentation update is needed because this ownership refactor does not change the supported environment-variable contract. The change is below 500 LOC, including tests, so no RFC is required.

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Codex assisted with ownership and lifecycle inventory, compatibility analysis, implementation, focused tests, verification, overlap checks, and pre-submission review. The human submitter reviewed every changed line and can defend the change end-to-end.

Signed-off-by: Schatten <czhengt@qq.com>
bool use_https = true;
std::string request_checksum_calculation;
std::string response_checksum_validation;
int64_t connect_timeout_ms = 10000;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use std::chrono::milliseconds instead of int64_t here?

std::string request_checksum_calculation;
std::string response_checksum_validation;
int64_t connect_timeout_ms = 10000;
int64_t request_timeout_ms = 30000;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above

Signed-off-by: Schatten <czhengt@qq.com>

@Aionw Aionw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Aionw
Aionw merged commit f49a559 into kvcache-ai:main Sep 17, 2026
26 checks passed
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