Skip to content

[Store] Structure RedisConnectionConfig environment settings - #4036

Merged
Aionw merged 2 commits into
kvcache-ai:mainfrom
bitborne:codex/redis-connection-environ-convergence
Sep 14, 2026
Merged

Aionw merged 2 commits into
kvcache-ai:mainfrom
bitborne:codex/redis-connection-environ-convergence

Conversation

@bitborne

@bitborne bitborne commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Description

Converges Mooncake Store's Redis connection environment settings under a private RedisConnectionConfig as part of #3809.

  • Groups MC_REDIS_DB_INDEX, MC_REDIS_USERNAME, and MC_REDIS_PASSWORD in the common environment catalog.
  • Resolves one config per Store Redis connection attempt and removes direct environment reads from the Redis connection business component, while keeping the public ConnectRedis() and ResolveRedisDbIndex() APIs unchanged.
  • Preserves unset and empty DB index as database 0, decimal indices from 0 through 255, surrounding ASCII whitespace, a leading +, exact credential text, password-gated AUTH, optional ACL username, binary-safe %b commands, AUTH-before-SELECT ordering, existing error mapping, and Redis-disabled behavior.
  • Keeps MC_REDIS_DB_INDEX string-typed in the catalog only to distinguish an explicitly empty value from a nonempty malformed value, then reuses the shared typed integer parser for nonempty input.
  • Intentionally rejects non-integer prefixes and suffixes such as 1junk, 1e2, and 0x1, which the previous std::stoll call accepted by ignoring unconsumed text. The documented contract is an integer from 0 through 255, and the invalid-value path remains silent.
  • Adds focused config, compatibility-wrapper, repeated-read, credential, error, and Redis-enabled business-path tests.

Open PR #1538 also touches these variables as part of a broad cross-module global Environ singleton migration. This PR is the narrow Store-only owner migration required by the current #3809 rules: it uses a component-owned private config, preserves Store diagnostics and per-connection lifecycle, adds direct RED-to-GREEN coverage, and leaves Transfer Engine and TENT readers unchanged.

The optional authentication, DB selection, binary-safe command, and ACL username behaviors established by #512, #1339, #1722, and #1757 remain unchanged.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • 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
cmake --build build --target redis_connection_config_test mooncake_store_master_objects -j32
ctest --test-dir build -N -R '^redis_connection_config_test$'
ctest --test-dir build -R '^redis_connection_config_test$' --output-on-failure
pre-commit run --files mooncake-common/include/environment_variables.h mooncake-store/src/CMakeLists.txt mooncake-store/src/config/redis_connection_config.h mooncake-store/src/config/redis_connection_config.cpp mooncake-store/src/ha/common/redis/redis_connection.cpp mooncake-store/tests/CMakeLists.txt mooncake-store/tests/redis_connection_config_test.cpp
./scripts/code_format.sh --check --changed-lines --base kvcache/main

Before the production change, the focused test passed the three preserved DB-index cases and failed the typed-contract regression for all three old prefix-acceptance inputs: 1junk, 1e2, and 0x1. After the change, the Redis-enabled config test passes, the Store master objects build successfully, all applicable touched-file pre-commit hooks pass with pre-commit 4.6.2, and the post-commit changed-lines check passes with clang-format 20.1.8.

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: the supported contract remains an integer DB index from 0 through 255, and the normalization only removes undocumented partial-number parsing.

AI Assistance Disclosure

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

Codex assisted with behavior and ownership inventory, compatibility analysis, implementation, focused RED-to-GREEN tests, verification, and pre-submission review. I reviewed every changed line and can defend the change end-to-end.

Signed-off-by: Schatten <czhengt@qq.com>
@Aionw
Aionw merged commit d642e9f into kvcache-ai:main Sep 14, 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