clean up Service::InvokeCmd

[romange]

The goal is to be able to allow asynchronous invocations of a command. Right now everything is a mess and it's very hard to analyse. I will perform refactoring and clean ups with the logic I understand, but I need help with the logic I am less familiar with.

1. VerifyCommandExecution should not be there. for multi/exec txs, move all verification checks to multi and similarly to how we fail the command if it has wrong arguments already during multi, we should fail the command if it fails VerifyCommandExecution. The exec should fail with EXECABORT if any of the commands inside multi fail the verification.
2. Similarly with OOM checks - not clear to me why it is in Service::InvokeCmd and not inside DispatchCommand together with other checks - and inside multi to prevent the whole tx to execute if any of the commands should be rejected. @BorysTheDev - who recently touched this code to reject slot migrations. Why OOM is handled here and not in DispatchCommand?
3. tracking code is triggered even if tracking is disabled - rewrite the code in such way that it will be dependent on a single tracking boolean - I do not want to see unrelated code called on the common path:

 tx->SetTrackingCallback({});`
....
 `if ((!tx && cid_name != "MULTI") || (tx && !tx->IsMulti())) {

[romange]

Also, what's up with if (cmd_cntx->cid->name() == "REPLCONF" && absl::EqualsIgnoreCase(ArgS(tail_args, 0), "ACK")) { inside InvokeCmd? Why do we need it and why there?

[dranikpg]

Similarly with OOM checks - not clear to me why it is in Service::InvokeCmd and not inside DispatchCommand together with other checks - and inside multi to prevent the whole tx to execute if any of the commands should be rejected.

And what if the multi tx is too large? We currently throw OOM even inside a multi/script/etc. All commands go through InvokeCmd, but not through DispatchCommand, so that's why its checked there

[dranikpg]

VerifyCommandExecution should not be there. for multi/exec txs, move all verification checks to multi and similarly to how we fail the command if it has wrong arguments already during multi, we should fail the command if it fails VerifyCommandExecution. The exec should fail with EXECABORT if any of the commands inside multi fail the verification.

Its high level verification of what we can tell without running the command. If you add a command after multi, it can get rejected, so we have to perform the filtering

[dranikpg]

Also, what's up with if (cmd_cntx->cid->name() == "REPLCONF" && absl::EqualsIgnoreCase(ArgS(tail_args, 0), "ACK")) { inside InvokeCmd? Why do we need it and why there?

It was a bug I remember some time ago and someone posted this short fix

[romange]

yeah, what was the bug. @kostasrim probably knows.

[romange]

without

I do not understand what you wrote. Why can not we verify while enquing into multi? what additional information do we have during exec?

[kostasrim]

From git blaming myself it was part of https://github.com/dragonflydb/dragonfly/issues/2668

ACKS could fail because of changes to ACL. We should not reply to ACKS and hence we skip the error reply to the client

[dranikpg]

what additional information do we have during exec?

The one that is relevant when EXEC actually executes: memory, acl, data store state, etc.