fix: Use MOVED error type for moved replies
#4125
Conversation
| @@ -1279,7 +1279,7 @@ void BZPopMinMax(CmdArgList args, Transaction* tx, SinkReplyBuilder* builder, | |||
| case OpStatus::TIMED_OUT: | |||
| return rb->SendNullArray(); | |||
| case OpStatus::KEY_MOVED: { | |||
| auto error = cluster::SlotOwnershipErrorStr(*tx->GetUniqueSlotId()); | |||
| auto error = cluster::SlotOwnershipError(*tx->GetUniqueSlotId()); | |||
There was a problem hiding this comment.
(unrelated to this change)
must say I do not understand the optional return type here. Why do we check for cluster config inside SlotOwnershipError and return optional if KEY_MOVED can only be due to cluster config. better CHECK for cluster config there and remove optional and remove CHECKs like CHECK(error.has_value());
There was a problem hiding this comment.
I think that the reason is to have the if-cluster-enabled and also if-slot-owned in the same place, and reuse it many times (currently 3 times)
There was a problem hiding this comment.
When I wrote this logic I didn't want to include additional headers into cluster_defs file, because it is included quite often, so created a string instead of ErrorReply. But if you want to return Reply we can remove the optional, because ErrorReply contains OK status. Also I think Roman is right and we can remove checking for cluster_config
There was a problem hiding this comment.
I've changed the return type to ErrorReply, feel free to follow up on the cluster config check :)
**The problem:** When in cluster mode, `MOVED` replies (which are arguably not even errors) are aggregated per slot-id + remote host, and displayed in `# Errorstats` as such. For example, in a server that does _not_ own 8k slots, we will aggregate 8k different errors, and their counts (in memory). This slows down all `INFO` replies, takes a lot of memory, and also makes `INFO` replies very long. **The fix:** Use `type` `MOVED` for moved replies, making them all the same under `# Errorstats` Fixes #4118
The problem:
When in cluster mode,
MOVEDreplies (which are arguably not even errors) are aggregated per slot-id + remote host, and displayed in# Errorstatsas such. For example, in a server that does not own 8k slots, we will aggregate 8k different errors, and their counts (in memory).This slows down all
INFOreplies, takes a lot of memory, and also makesINFOreplies very long.The fix:
Use
typeMOVEDfor moved replies, making them all the same under# ErrorstatsFixes #4118