Repository navigation
Conversation
|
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/tasks/,cloud/storage/ (test time: 259s): all tests PASSED for commit 946bfb7.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 248s): all tests PASSED for commit 946bfb7.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1241s): some tests FAILED for commit 946bfb7.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 18s): all tests PASSED for commit 946bfb7.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4462s): all tests PASSED for commit 946bfb7.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946bfb74c0
ℹ️ 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".
qkrorlqr
left a comment
There was a problem hiding this comment.
are you planning to implement this logic separately for CreateFileSystem requests?
| .Set(shardIndex); | ||
|
|
||
| auto request = | ||
| std::make_unique<TEvIndexTablet::TEvUnsafeChangeTabletStateRequest>(); |
There was a problem hiding this comment.
we probably need some kind of a ChangeStateCookie in this request to avoid ChangeState-races for concurrent requests from multiple filestore-servers
e.g. use ChangeStateCookie = hash(PrevState) to be able to check in the tablet code that the caller actually knows the most recent state
There was a problem hiding this comment.
With ResizeState.Version and updated Merge/Verify/Retry logic in TAlterFileStoreActor::HandleResizeStateResponse protect us from races.
- Actors A and B both read ResizeState.Version = 1.
- A creates shard X and sends bitmap {X} with Version = 1.
- B creates shard Y and sends bitmap {Y} with Version = 1.
- The tablet handles e.g. A first, stores bitmap {X}, and bumps Version to 2.
- The tablet handles B next. B's Version = 1 is stale, so the tablet does not
apply bitmap {Y}. The response contains the current persisted state:
Version = 2, bitmap {X}. - B merges current persisted state with its own local state, and if this merged state differs from persisted state, does retry to store bitmap {Y'}
There was a problem hiding this comment.
Anyway. When two concurrent resize requests were started, both actors have the same initial topology, and they both tries to create shard{curSize+1}. Where only one actor wins, another got StatusAlreadyExists error and stop processing.
| CreateShard(ctx, NextShardToCreate); | ||
| ++NextShardToCreate; | ||
| ++requests; | ||
| } |
There was a problem hiding this comment.
Maybe it would be less error-prone to keep this code here as well:
if (ShardsToCreate == 0) {
ConfigureShards(ctx);
return;
}The caller code could also be simplified this way.
There was a problem hiding this comment.
I would keep single responsibility for ContinueCreateShards method.
We want to run ConfigureShards only if no create calls were initiated by this actor. I.e. if there are in-flight requests to update bitmap, we want first to wait until it's stored durably.
There was a problem hiding this comment.
Can we add early Resize/Alter retries to our medium tests?
For example, we can add them to this test, which we use for feature adoption: https://github.com/ydb-platform/nbs/blob/main/cloud/filestore/tests/scheme_cache/test.py
It would also be good to run two filestore-server instances and send Resize retries to them in a round-robin manner.
|
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/tasks/,cloud/storage/ (test time: 267s): all tests PASSED for commit fa93d9a.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 260s): all tests PASSED for commit fa93d9a.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1426s): all tests PASSED for commit fa93d9a.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4628s): some tests FAILED for commit fa93d9a.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4237s): some tests FAILED for commit fa93d9a.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4212s): some tests FAILED for commit fa93d9a.
|
fa93d9a to
3107f78
Compare
|
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/tasks/,cloud/storage/ (test time: 274s): all tests PASSED for commit 3107f78.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 333s): all tests PASSED for commit 3107f78.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1605s): some tests FAILED for commit 3107f78.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 46s): all tests PASSED for commit 3107f78.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4872s): all tests PASSED for commit 3107f78.
|
3107f78 to
749d57f
Compare
|
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/tasks/,cloud/storage/ (test time: 275s): all tests PASSED for commit 749d57f.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 324s): all tests PASSED for commit 749d57f.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1689s): some tests FAILED for commit 749d57f.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 28s): all tests PASSED for commit 749d57f.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5348s): some tests FAILED for commit 749d57f.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 643s): some tests FAILED for commit 749d57f.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 639s): some tests FAILED for commit 749d57f.
|
I want to keep this PR explicitly for |
|
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/tasks/,cloud/storage/ (test time: 273s): all tests PASSED for commit 2a3c7c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 315s): all tests PASSED for commit 2a3c7c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1482s): all tests PASSED for commit 2a3c7c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4923s): all tests PASSED for commit 2a3c7c2.
|
aee64bb to
7175627
Compare
|
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.
|
|
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.
|
|
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.
|
|
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/tasks/,cloud/storage/ (test time: 276s): all tests PASSED for commit d0efd0f.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 318s): all tests PASSED for commit d0efd0f.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1535s): all tests PASSED for commit d0efd0f.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4919s): all tests PASSED for commit d0efd0f.
|
| TEvIndexTablet::TEvConfigureShardsResponse, | ||
| HandleConfigureMainFileStoreResponse); | ||
| HFunc( | ||
| TEvIndexTablet::TEvUnsafeChangeTabletStateResponse, |
There was a problem hiding this comment.
Originally the Unsafe* set of operations was intended for manual interventions into the tablet state. Routing ordinary requests through it is a bit unusual
There was a problem hiding this comment.
Decided to use Usafe after a talk with @qkrorlqr. Reasoning - reuse already existing API to avoid reserving new operation IDs just to get/put state of the tablet.
Probably, we may rename this operation to just ChangeTabletState.
I'm not sure if this could cause any incompatibilities during rolling updates?
| } | ||
| } | ||
|
|
||
| bool TAlterFileStoreActor::HasUnpersistedCreatedShards() const |
There was a problem hiding this comment.
This is identical code across two actors. Probably worth unifying into a common method
|
|
||
| message TCompressedBitmapChunk | ||
| { | ||
| uint32 ChunkIdx = 1; |
There was a problem hiding this comment.
I might be missing something, but what is the value of chunking here as opposed to just single bytes or repeated bool?
| "[%s] UnsafeChangeTabletState failed: %s", | ||
| LogTag.c_str(), | ||
| FormatError(msg->GetError()).Quote().c_str()); | ||
| return; |
There was a problem hiding this comment.
-
We can get a retriable error in case of an ongoing tablet restart. It is probably worth retrying retriable errors (see
GetErrorKindfunction) -
If we reach this place, won't this actor just leak even without replying to the caller?
-
It is both reasonable to eeither propagate non-retiriable error to the caller, or raise a critical event if this is supposed to be an Impossible event
| } | ||
|
|
||
| if (shardCreationState.GetVersion() < ShardCreationStateVersion) { | ||
| return; |
There was a problem hiding this comment.
Will this line also lead to an actor leak?
|
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/tasks/,cloud/storage/ (test time: 273s): all tests PASSED for commit ee413d7.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 356s): all tests PASSED for commit ee413d7.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1579s): all tests PASSED for commit ee413d7.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5013s): all tests PASSED for commit ee413d7.
|
|
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/tasks/,cloud/storage/ (test time: 272s): all tests PASSED for commit 35411cf.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 308s): all tests PASSED for commit 35411cf.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1519s): all tests PASSED for commit 35411cf.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4837s): all tests PASSED for commit 35411cf.
|
35411cf to
7f35614
Compare
Notes
The document describes the main resize state machine, shard creation throttling, persistent shard creation state, CAS-based bitmap update handling, retry behavior, and the reason why shard creation state is retained after a successful resize. This should make the upcoming implementation PRs easier to review and preserve the intended invariants around retry-safe shard creation.
Stack