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

Let shuffle always use get_next_message to support holey outboxes #1640

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

tillrohrmann
Copy link
Contributor

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.

@tillrohrmann
Copy link
Contributor Author

This PR needs to be backported to release-1.0 and ideally also release-0.9 to verify with the user that it fixes the problem.

@AhmedSoliman
Copy link
Contributor

We might need to measure the performance impact of this change.

@tillrohrmann
Copy link
Contributor Author

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.

@tillrohrmann
Copy link
Contributor Author

Running cargo bench --bench throughput_parallel:

This PR:
throughput/parallel     time:   [650.60 ms 664.77 ms 680.18 ms]
                        thrpt:  [5.8808 Kelem/s 6.0171 Kelem/s 6.1481 Kelem/s]
main:
throughput/parallel     time:   [643.00 ms 656.26 ms 670.83 ms]
                        thrpt:  [5.9627 Kelem/s 6.0951 Kelem/s 6.2208 Kelem/s]

Running cargo bench --bench throughput_sequential:

This PR:
throughput/sequential   time:   [639.37 µs 645.52 µs 652.83 µs]
                        thrpt:  [1.5318 Kelem/s 1.5491 Kelem/s 1.5640 Kelem/s]
main:
throughput/sequential   time:   [625.67 µs 628.62 µs 632.37 µs]
                        thrpt:  [1.5813 Kelem/s 1.5908 Kelem/s 1.5983 Kelem/s]

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.

Copy link
Contributor

@AhmedSoliman AhmedSoliman left a 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.
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.
@tillrohrmann tillrohrmann merged commit 1313e49 into restatedev:main Jun 20, 2024
5 checks passed
@tillrohrmann tillrohrmann deleted the issues/1639 branch June 20, 2024 12:21
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

Successfully merging this pull request may close these issues.

Shuffler can get stuck when having holes in the outbox
2 participants