dragonflydb / dragonfly

A modern replacement for Redis and Memcached
https://www.dragonflydb.io/
Other
25.96k stars 954 forks source link

We need to limit the number of squashed commands when running in pipeline mode #4128

Open BorysTheDev opened 1 week ago

BorysTheDev commented 1 week ago

4002 depends on the number of squashed commands because we store temporary results of the squashed commands until we execute them all.

BorysTheDev commented 4 days ago

@romange We have discussed with @adiholden simply limiting the number of squashing commands and to my mind it doesn't give us any significant benefits. I have suggested breaking the HOP if we have enough output, sending data, and then starting a new HOP. What do you think about it?

@adiholden told me that I can do it in the next way: change MultiCommandSquasher::SquashedHopCb to run invoke command in the loop but then break if CapturingReplyBuilder exceeded some size, then return how many commands were actually executed and run them after sending the answer to a user

romange commented 4 days ago

@BorysTheDev why limiting the squashing commands does not provide benefits?

BorysTheDev commented 4 days ago

@romange because we really depends on the command response and we can have 30 commands squashed with the response of 3GB or 1000 commands squashed with a 50KB response

romange commented 4 days ago

So the global limit will be beneficial for some cases and for some exteme cases it won't be very efficient. I agree.

But since limiting squashing post factum, when you already performed operations is more complicated than limiting commands in advance, I prefer we do the simple approach first and quickly and then improving it if needed. We have datastore in prod that needs fixing fast.

BorysTheDev commented 23 hours ago

The current limitation is 32 squashed commands per shard. So the only enhancement we can do is breaking the HOP if we have enough output.

I've removed the urgent label because it doesn't look like high priority task now