refactor: remove is_oom flag from ConnectionContext #5216
Conversation
BorysTheDev
requested review from
mkaruza and
kostasrim
and removed request for
mkaruza
BorysTheDev
force-pushed
the
refactor_command_context
branch
from
43c0949 to
fe4d813
Compare
There was a problem hiding this comment.
What I would have considered is to change the return type of InvokeCmd from bool to an enum (no error code as it has virtual members -- even for SystemCategory as it's basically a static global object and not a thread local). And then I would propagate the error (if any) from DispatchCommand back to the caller. My rationale for this is:
- We already return if we succeed or not from InvokeCmd. Why not just supplement that?
CommandContextbelongs logically withinDispatchCommandbecause this is where we have all the information to create it. Seedfly_cntx->transaction = dist_trans.get();. Now we overwritedfly_cntx&& theCommandContextpassed toDispatchCommandand I find it slightly confusing that we do the extra bookeeping
This is not something I gave much thought but rather thinking out loud here
|
@kostasrim I need these changes not only for OOM, but for cluster slots too. I agree that this is also not the best solution, but better than before. Let's consider InvokeResult separately. |
I think that for cluster slots you already have the slot information inside the Transaction class and therefore you dont need to add it to CommandCntx. you can have the tx->GetDbContext() return the slots data as well. For this PR change I think that you dont need the is_oom in CommandCntx. If you can access the conn_context_->transaction->local_result_ after calling dispatch command in JournalExecutor::Execute |
|
@adiholden I want to remove the cluster code from the transaction, because we do slot calculations several times even before transaction |
|
@kostasrim I like your idea regarding InvokeCmd. If you want, I can remove OOM refactoring from this PR, but I still need other changes regarding CommandContext. Or we can merge the current refactoring and, in the future, return to your idea regarding the InvokeCmd result. I don't want to do these changes here because in this case the PR will have 2 absolutely different refactorings. |
BorysTheDev
force-pushed
the
refactor_command_context
branch
3 times, most recently
from
9371eb4 to
46060b7
Compare
BorysTheDev
force-pushed
the
refactor_command_context
branch
from
46060b7 to
112afdd
Compare
BorysTheDev
changed the title
| @@ -143,12 +143,13 @@ class ClusterShardMigration { | |||
| } | |||
|
|
|||
| private: | |||
| void ExecuteTx(TransactionData&& tx_data, ExecutionState* cntx) { | |||
| std::error_code ExecuteTx(TransactionData&& tx_data, ExecutionState* cntx) { | |||
There was a problem hiding this comment.
Nice! We need the same for replication ;)
fixes: #5196