[Filestore] issue-5432: add basic async open support (tablet)#6589
[Filestore] issue-5432: add basic async open support (tablet)#6589budevg wants to merge 2 commits into
Conversation
budevg
changed the title
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: 4891s): all tests PASSED for commit 520c018.
|
|
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: 7164s): all tests PASSED for commit 520c018.
|
|
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: 4727s): all tests PASSED for commit 520c018.
|
|
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 520c018.
|
|
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: 4914s): all tests PASSED for commit 520c018.
|
![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: 520c0189ab
ℹ️ 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".
| const bool safeAsync = hasLocalHandle && | ||
| isReadOnly && | ||
| Config->GetAsyncCreateHandleEnabled() && | ||
| args.Request.GetAllowAsyncCreateHandle(); |
There was a problem hiding this comment.
When AsyncCreateHandleEnabled is enabled but neither async-destroy flag is, the tablet can still return HandleCreatedAsync to any client that sets AllowAsyncCreateHandle. If that client then closes with a normal DestroyHandle before the queued ConfirmCreateHandle runs, the close removes the handle and the later confirm recreates it, leaking an open handle; the FUSE side already guards against this exact ordering, but the server-side decision should enforce the same requirement before advertising an async-created handle.
Useful? React with 👍 / 👎.
| xxx(ReadData, __VA_ARGS__) \ | ||
| xxx(WriteData, __VA_ARGS__) \ | ||
| xxx(AllocateData, __VA_ARGS__) \ | ||
| xxx(ConfirmCreateHandle, __VA_ARGS__) \ |
There was a problem hiding this comment.
This macro feeds the numeric EFileStoreRequest enum that is written to profile/event logs, so inserting ConfirmCreateHandle here shifts Fsync, FsyncDir, endpoint, and private request ids (the updated dump test now expects Fsync to move from 46 to 47). Existing logs and any consumers that decode stored request type numbers will be mislabelled after this change; add the new request without renumbering the existing values.
Useful? React with 👍 / 👎.
|
|
||
| TIndexTabletDatabase db(tx.DB); | ||
|
|
||
| if (auto* handle = FindHandle(args.Handle)) { |
There was a problem hiding this comment.
do we even need to start a transaction if the requested handle has already been created? seems that we can check it and reply directly from HandleConfirmCreateHandle
yes, we'll still need to doublecheck here (because there might be a concurrent request attempting to create the same handle), but seems that we can make the happy path much faster
|
|
||
| auto id = CreateNode(tablet, TCreateNodeArgs::File(RootNodeId, "test")); | ||
|
|
||
| // Hold the create tx commit so the async response is acknowledged |
There was a problem hiding this comment.
I understand the purpose of the test but I don't understand this comment - it says that something's acknowledged before SessionHandle becomes durable but in this test the feature is disabled so nothing is acknowledged before becoming durable
| return; | ||
| } | ||
| } else { | ||
| handle = UnsafeCreateHandle( |
There was a problem hiding this comment.
now the comment and the name of this method are misleading:
// This method is unsafe because it accepts a handleId that may not follow the
// conventions enforced by TIndexTabletState::GenerateHandle. We need this
// method for testing purposes, such as verifying the response to handles with
// an incorrect shard number.
I suggest to add something like NProto::TError RegisterHandle(db, session, handleId, nodeId, commitId, flags) instead which would take args.NodeId as input and would do the NodeId check inside it instead. This way:
Unsafestate methods will still be used only byUnsafeprivate api handlers- the other methods would be safe in the sense that they would do the necessary checks
| ? createRequest.GetHeaders().GetRequestId() | ||
| : callContext->RequestId; | ||
|
|
||
| THandleOpsQueue::EResult result; |
There was a problem hiding this comment.
just curious, do we really want to handle destroy/create from the single queue?
3 participants
#5432