-
Notifications
You must be signed in to change notification settings - Fork 44
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
Let shuffle always use get_next_message to support holey outboxes #1640
Conversation
This PR needs to be backported to |
We might need to measure the performance impact of this change. |
I can run both versions (before and after) with the parallel throughput benchmark to assess its effect on performance. I'll report the numbers once I have them. |
Running
Running
Both benchmarks were run on my local M1Pro (note there was a bit of variance). As one can see, there is slight degradation in performance. I think the degradation is not severe enough for not shipping the hotfix. Once we have #1641 implemented, this regression should be gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
WipeMode::All will now delete the whole node base dir to also remove a cluster marker file. This fixes restatedev#1629.
c81bf39
to
3ff690d
Compare
This commit changes the shuffle to always use get_next_message to read the next outbox message. Before we were using get_message which was only looking at a specific outbox entry. If this outbox entry was empty, then the shuffle assumed that the outbox is empty. This did not work if the outbox contained holes. Now with get_next_message, we always scan until the next outbox message. The change itself is trivial. In order to ensure that the change works, this commit added a few unit tests to ensure the desired behaviour. Note: With replacing get_message with get_next_message, the shuffle will perform for every read a RocksDB scan operation. This is highly inefficient and we should replace this logic with a tailing iterator. This fixes restatedev#1639.
3ff690d
to
1313e49
Compare
This commit changes the shuffle to always use get_next_message to read the next outbox message. Before we were using get_message which was only looking at a specific outbox entry. If this outbox entry was empty, then the shuffle assumed that the outbox is empty. This did not work if the outbox contained holes. Now with get_next_message, we always scan until the next outbox message.
The change itself is trivial. In order to ensure that the change works, this commit added a few unit tests to ensure the desired behaviour.
Note: With replacing get_message with get_next_message, the shuffle will perform for every read a RocksDB scan operation. This is highly inefficient and we should replace this logic with a tailing iterator.
This fixes #1639.