chore(string_family): Refactor to prep for adding the GAT command #5289
Conversation
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
A couple of functions are extracted from the MGet command. The reason for this is that in a follow up PR, the GAT command will be added which will reuse some of the functions: A function to collect keys is extracted. This takes a function as input which will return an iterator for one key. The iterator can be mutable or immutable, for MGet it is immutable. For GAT it will be mutable. A check is added such that a read-only command does not use a function returning a mutable iterator. Another function is created to reorder collected results based on the shards of the keys and the original order of the keys in the command. Signed-off-by: Abhijat Malviya <[email protected]>
Signed-off-by: Abhijat Malviya <[email protected]>
romange
reviewed
Jun 14, 2025
Comment on lines
+556
to
+560
| const auto cid = t->GetCId(); | ||
| constexpr bool is_mut_iter = std::is_same_v<Iter, DbSlice::Iterator>; | ||
| DCHECK(!(cid->IsReadOnly() && is_mut_iter)) | ||
| << "mutable iterator used with read-only command " << cid->name(); | ||
|
|
There was a problem hiding this comment.
Suggested change
| const auto cid = t->GetCId(); | |
| constexpr bool is_mut_iter = std::is_same_v<Iter, DbSlice::Iterator>; | |
| DCHECK(!(cid->IsReadOnly() && is_mut_iter)) | |
| << "mutable iterator used with read-only command " << cid->name(); | |
| constexpr bool is_mut_iter = std::is_same_v<Iter, DbSlice::Iterator>; | |
| if constexpr (is_mut_iter) { | |
| const auto* cid = t->GetCId(); | |
| DCHECK(!cid->IsReadOnly()) << "mutable iterator used with read-only command " << cid->name(); | |
| } |
|
|
||
| template <typename Iter> | ||
| MGetResponse CollectKeys(BlockingCounter wait_bc, uint8_t fetch_mask, const Transaction* t, | ||
| EngineShard* shard, SearchKey<Iter> find_op) { | ||
| ShardArgs keys = t->GetShardArgs(shard->shard_id()); | ||
| DCHECK(!keys.Empty()); | ||
|
|
||
| auto& db_slice = t->GetDbSlice(shard->shard_id()); |
There was a problem hiding this comment.
move db_slice to the block where you use it
| @@ -1294,12 +1319,41 @@ void StringFamily::DecrBy(CmdArgList args, const CommandContext& cmnd_cntx) { | |||
| return IncrByGeneric(key, -val, cmnd_cntx.tx, cmnd_cntx.rb); | |||
| } | |||
|
|
|||
| absl::FixedArray<optional<GetResp>, 8> ReorderShardResults( | |||
There was a problem hiding this comment.
absl::FixedArray<optional<GetResp>, 8> is more than 512 bytes - and it's not moveable, meaning that now we always have to copy at least 512 bytes.
Instead, I suggest the following signature:
void ReorderShardResults(const std::vector<MGetResponse>& mget_resp, const Transaction* t, absl::FixedArray<optional<GetResp>, 8>* dest);
and define res out side of the function.
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.
A couple of functions are extracted from the MGet command. In a follow up PR, the GAT command will be added which
will reuse some of the functions:
For context, the original PR is #5257