[Filestore] issue-5432: add basic async open support#6580
Conversation
budevg
added
large-tests
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-msan have completed. Tip Planned checks for linux-x86_64-release-msan.
🔴 linux-x86_64-release-msan target: cloud/filestore/ (test time: 4947s): some tests FAILED for commit 7198199.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7225s): all tests PASSED for commit 7198199.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
🟢 linux-x86_64-release-asan target: cloud/filestore/ (test time: 5056s): all tests PASSED for commit 7198199.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-ubsan have completed. Tip Planned checks for linux-x86_64-release-ubsan.
🟢 linux-x86_64-release-ubsan target: cloud/filestore/ (test time: 4626s): all tests PASSED for commit 7198199.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
🟢 linux-x86_64-release-tsan target: cloud/filestore/ (test time: 5667s): all tests PASSED for commit 7198199.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7198199fae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| xxx(ReadData, __VA_ARGS__) \ | ||
| xxx(WriteData, __VA_ARGS__) \ | ||
| xxx(AllocateData, __VA_ARGS__) \ | ||
| xxx(ConfirmCreateHandle, __VA_ARGS__) \ |
There was a problem hiding this comment.
Adding ConfirmCreateHandle inside FILESTORE_DATA_METHODS renumbers every later EFileStoreRequest value, because FILESTORE_REQUESTS expands this macro before Fsync, FsyncDir, endpoint requests, and the private request names. Those numeric ids are persisted in profile log records and decoded by tools such as filestore-profile-tool dumpevents, so after this change existing records with the old Fsync/FsyncDir/etc. ids will be reported under the wrong request names. Please append the new request after the existing ids or assign stable explicit values instead of shifting the current sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I considered preserving the existing numeric values. With the current macro layout, doing so would require introducing a separate macro, such as FILESTORE_EXTRA_DATA_METHODS, and appending it explicitly after all existing EFileStoreRequest entries.
This is somewhat awkward because every future service method added after the existing request list would need its own extra macro and explicit enum/name-array entry to avoid renumbering the persisted request types.
Since this change only renumbers the small trailing group of request types (Fsync, FsyncDir, GetSessionEventsStream, endpoint requests, and the private blob-related requests), I considered that a reasonable tradeoff for keeping the service-method declarations in one natural list.
| if (safeAsync) { | ||
| args.Response.SetHandleCreatedAsync(true); |
There was a problem hiding this comment.
When safeAsync is true, this flag is set only after AddDupCacheEntry has already stored args.Response in the duplicate cache above. If the original async CreateHandle reply is lost and the client retries the same request id before the transaction/handle is durable, the retry is served from that cache without HandleCreatedAsync, so the client will not enqueue ConfirmCreateHandle and the returned handle can disappear after a tablet restart. Set the flag before adding the duplicate-cache entry, or update the cached response, so duplicate replies preserve the confirmation requirement.
Useful? React with 👍 / 👎.
| f"exec sudo bash -c 'python3 {script} " | ||
| f"hold {OPEN_HANDLE_COUNT} {READY_PATH} {RELEASE_PATH} " | ||
| f"{VERIFY_PATH} {VERIFIED_PATH} {UNLINK_PATH} {UNLINKED_PATH} " | ||
| f">{LOG_PATH} 2>&1'"), |
There was a problem hiding this comment.
This command is executed by ssh inside the QEMU guest, but LOG_PATH is a host-side common.output_path(...); the QEMU recipe mounts the source/build paths into the guest, not the test output directory. When the guest shell tries to apply this redirection before starting python3, the parent directory is absent in the guest and the workload exits immediately, so the async-open test never reaches the open/confirm checks. Use a guest path such as /tmp/async_open.log and copy it back if needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The QEMU recipe mounts the entire ya build root into the guest at the same path mount setup. Since common.output_path() returns a path in the test output directory, which is under that build root, LOG_PATH is accessible from the guest. Keeping the log there also retains it as a test artifact after completion.
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7170s): some tests FAILED for commit 96ec39c.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 240s): all tests PASSED for commit 96ec39c.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
🟢 linux-x86_64-release-tsan target: cloud/filestore/ (test time: 5561s): all tests PASSED for commit 96ec39c.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-ubsan have completed. Tip Planned checks for linux-x86_64-release-ubsan.
🟢 linux-x86_64-release-ubsan target: cloud/filestore/ (test time: 4656s): all tests PASSED for commit 96ec39c.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-msan have completed. Tip Planned checks for linux-x86_64-release-msan.
🟢 linux-x86_64-release-msan target: cloud/filestore/ (test time: 4981s): all tests PASSED for commit 96ec39c.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
🟢 linux-x86_64-release-asan target: cloud/filestore/ (test time: 5053s): all tests PASSED for commit 96ec39c.
|
|
Let's split this PR into several PRs: add |
2 participants
#5432
#6486