Repository navigation
[TENT] Make UB batch submission all-or-nothing - #56
Open
Connor-Matthew wants to merge 2 commits into
Open
Connor-Matthew wants to merge 2 commits into
Connor-Matthew wants to merge 2 commits into
Conversation
UbTransport::submitTransferTasks() handed tasks to UbWorkers one at a time. When a later task was rejected, the earlier tasks of the same call were already queued and could only be asked to cancel, so a failed submit could still run part of the batch. UbWorkers::submit() also skipped any slice that could not be marked queued and reported success for the rest of the task. Add UbWorkers::submitBatch(). It checks every task and slice before changing any state, then marks and queues all slices under the queue lock. If a slice changes state, the workers stop, or the queue cannot grow, nothing is published, and any task whose slices were already marked is cancelled so no slice is left stuck in the queued state. submit() now goes through submitBatch(), and submitTransferTasks() submits the whole request list in one call before adding the tasks to the sub-batch. Arm the progress callback only after the tasks are in the sub-batch. A very small task could complete before it was queryable and its notification was lost; notify once after arming to cover that window. Co-authored-by: Le1zyCatt <2490162471@qq.com> Signed-off-by: Connor-Matthew <60215777+Connor-Matthew@users.noreply.github.com>
Roll back partial enqueues for allocation and length errors. Cancel marked tasks before constructing the returned error status. Inject a second-enqueue failure to verify worker rollback and preserve existing transport sub-batch task IDs after a failed submit. Signed-off-by: Connor-Matthew <60215777+Connor-Matthew@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Internal CI run for the UB batch-submission fix. Not for merging into the fork's main.
UbTransport::submitTransferTasks()handed tasks toUbWorkersone at a time. When a later task was rejected, the earlier tasks of the same call were already queued and could only be asked to cancel, so a failed submit could still run part of the batch.UbWorkers::submit()also skipped any slice that could not be marked queued and still reported success for the rest of the task.Changes:
UbWorkers::submitBatch(). It checks every task and slice before changing any state, then marks and queues all slices under the queue lock. If a slice changes state, the workers stop, or the queue cannot grow, nothing is published, and any task whose slices were already marked is cancelled so no slice is left stuck in the queued state.submit()now goes throughsubmitBatch().submitTransferTasks()submits the whole request list in one call, then adds the tasks to the sub-batch.This fix was split out of the TENT_4 branch (
ub/tent4-integration) and contains no receiver-credit code.Suggested Merge Priority
Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Local Linux arm64 container with the CI
ub-mockconfiguration (Debug,-DUSE_UB=ON), full build plus the TENT test directory.Test commands:
Test results:
transfer_engine_config_override_testfails only in the local container: its DNS resolves an intentionally invalid hostname, and the container has no NUMA sysfs. The same four cases failed there before this change, and this PR does not touch that code.tent_ub_native_data_path_testpasses 50 repeated runs.submitBatch()changed back to submitting tasks one at a time, the new workers-level test fails (one task queued, slice left in the queued state).Checklist
Signed-off-bytrailer certifying the DCO./scripts/code_format.shAI Assistance Disclosure
An AI coding assistant helped split this fix out of the TENT_4 branch, port the tests, run the local container build and mutation check, and draft this description. The human submitter is responsible for reviewing every changed line.