Skip to content

[Store] Structure FilereadWorkerPoolConfig environment settings - #4166

Merged
Aionw merged 1 commit into
kvcache-ai:mainfrom
bitborne:codex/fileread-worker-environ-convergence
Sep 17, 2026
Merged

Aionw merged 1 commit into
kvcache-ai:mainfrom
bitborne:codex/fileread-worker-environ-convergence

Conversation

@bitborne

@bitborne bitborne commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Description

Part of #3809. Extract MC_FILEREAD_WORKERS from the transfer-task source into a private, client-owned FilereadWorkerPoolConfig and the common environment catalog. The worker pool still takes one process-wide snapshot on first construction, defaults to 10 workers for absent/empty/invalid values, and emits the existing warning with the raw input for a nonempty invalid value. The unrelated NoF settings and the public worker-pool API stay unchanged.

The shared typed integer parser is used for nonempty input. This intentionally accepts surrounding ASCII whitespace (for example, "2 "), which the previous strtol path rejected; the supported positive integer behavior and first-use caching are covered by direct config and worker-count tests. This is a single-owner extraction following merged PRs #3872 and #4036.

Open PR #3424 also edits transfer_task.cpp while refactoring NoF internals, including the adjacent #ifdef USE_NOF boundary. It does not migrate MC_FILEREAD_WORKERS; if it merges before this PR, the NoF guard overlap needs resolving at integration time. This PR does not rebase or modify NoF behavior preemptively.

Module

  • Mooncake Store (mooncake-store)
  • Common (mooncake-common)

Type of Change

  • Refactor

How Has This Been Tested?

Test commands:

cmake --build build --parallel 232 --target fileread_worker_pool_config_test fileread_worker_pool_test transfer_task_test mooncake_store_client_objects
ctest --test-dir build -R '^(fileread_worker_pool_config_test|transfer_task_test)$' --output-on-failure
GTEST_FILTER='*-TransferTaskTest.BatchGetOffloadObjectCopiesPinnedHostToGpu' ctest --test-dir build -R '^(fileread_worker_pool_config_test|fileread_worker_pool_test|transfer_task_test)$' --output-on-failure
ctest --test-dir build -R '^fileread_worker_pool_test$' --repeat until-fail:10 --output-on-failure
./scripts/code_format.sh --check --staged
pre-commit run --files mooncake-common/include/environment_variables.h mooncake-store/src/CMakeLists.txt mooncake-store/src/config/fileread_worker_pool_config.cpp mooncake-store/src/config/fileread_worker_pool_config.h mooncake-store/src/transfer_task.cpp mooncake-store/tests/CMakeLists.txt mooncake-store/tests/fileread_worker_pool_config_test.cpp mooncake-store/tests/fileread_worker_pool_test.cpp

Test results:

  • Unit tests pass for the new config, isolated worker-pool lifecycle, and all non-GPU baseline-failing transfer-task cases (3/3 focused CTest targets with that one Google Test case filtered).
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

The unfiltered target has one pre-existing GPU pinned-host allocation failure: TransferTaskTest.BatchGetOffloadObjectCopiesPinnedHostToGpu gets a null buffer. The same test fails at the same assertion in the independently built branch with unmodified fileread worker code, and fails when run alone; it is not caused by this extraction. Before the production change, the new whitespace/worker-count test failed against the old parser; after the change its isolated binary passes 10 repetitions. The client object target builds, and touched-file pre-commit and staged-line clang-format 20 checks pass.

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: the public API and positive-integer contract remain the same; whitespace acceptance is described above.

AI Assistance Disclosure

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

Codex assisted with ownership analysis, implementation, RED-to-GREEN focused tests, build/format/hook verification, and review. The human submitter will review every changed line and is responsible for the change end to end.

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