Details

Reviewers

tomek
varun

Commits

rCOMM6896bb4bb392: [services] Tests - Backup - Send log

Summary

Depends on D4632

Adding a piece of test for "send log" functionality.

Test Plan

cd services
yarn run-performance-tests backup 1

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, varun.

• karol requested review of this revision.Jul 26 2022, 2:50 AM

tomek requested changes to this revision.Jul 26 2022, 5:31 AM

tomek added inline comments.

services/commtest/tests/backup_performance_test.rs
135 ↗	(On Diff #14925)	Why do we iterate `log_items` when we don't use the item, just the index? Should we iterate only up to the length?
141–147 ↗	(On Diff #14925)	Is this code properly formatted?
153 ↗	(On Diff #14925)	Why do we need to drop explicitly?
163 ↗	(On Diff #14925)	This doesn't seem like a useful log. Should we replace it by something more descriptive?

This revision now requires changes to proceed.Jul 26 2022, 5:31 AM

address feedback - remove redundant log and refactor the loop

services/commtest/tests/backup_performance_test.rs
135 ↗	(On Diff #14925)	We can iterate over length
141–147 ↗	(On Diff #14925)	Yes
153 ↗	(On Diff #14925)	I decided it's better to do it like that. An alternative may be (not sure if that would work) to replace `sender_cloned` with an original sender for example in the last iteration of the loop. The thing is the sender has to be dropped in order to unblock the receiver. Usually, we would move the sender into a separate thread when it would be dropped along with that thread. But here, we spawn a lot of threads in a loop so I think it's easier and clearer to just drop manually than make conditions in that loop and clone or pass the original conditionally.
163 ↗	(On Diff #14925)	This should be removed, thx.

A lot of cloning happening here. If we have data that requires shared ownership, we should use Arc

tomek added inline comments.

services/commtest/tests/backup_performance_test.rs
144 ↗	(On Diff #14962)	Maybe change this message a little bit. The notation `X/Y` suggests that it's Xth item from Y, but it is not the case. Be more explicit and state that these numbers are backup and log indices.
153 ↗	(On Diff #14925)	It feels wrong to use `drop` for this purpose - I think it might be better to avoid it. But at the same time, other solutions like introducing new scope are significantly more complicated. So I think we can keep it, but please add a comment explaining the purpose - ideally with a link to the docs.

This revision is now accepted and ready to land.Jul 27 2022, 4:41 AM

services/commtest/tests/backup_performance_test.rs
153 ↗	(On Diff #14925)	People use it like it's not some sort of anti-pattern: https://stackoverflow.com/a/66470228 https://stackoverflow.com/questions/31391791/closing-a-channel-like-in-go In the docs they say: The channel is closed when all senders have been dropped, or when close is called. I don't think we can call `close` explicitly, because we'd have to intentionally wait for all the threads to terminate. Cloning a sender, dropping the original one, and letting it wait for all the senders to be dropped automatically is the best option I can see here. I'm going to add a comment, although it looks like a common pattern in the Rust world.