Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BorysTheDev opened this issue Nov 13, 2024 · 5 comments
Assignees

Comments

@BorysTheDev
Copy link
Contributor

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

@romange romange added urgent Important issue that needs to be fixed asap Next Up task that is ready to be worked on and should be added to working queue labels Nov 17, 2024
@BorysTheDev
Copy link
Contributor Author

@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
Copy link
Collaborator

romange commented Nov 17, 2024

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

@BorysTheDev
Copy link
Contributor Author

@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
Copy link
Collaborator

romange commented Nov 17, 2024

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 BorysTheDev removed urgent Important issue that needs to be fixed asap Next Up task that is ready to be worked on and should be added to working queue labels Nov 21, 2024
@BorysTheDev
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants