Details

Reviewers

tomek
varun
ashoat

Commits

rCOMM4605babb58c1: [services] Tests - Add send log

Summary

Depends on D4165

Adding a script for sending log.

Test Plan

This, for now, doesn't do anything.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.May 31 2022, 6:50 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptMay 31 2022, 6:50 AM

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 6:57 AM

Harbormaster failed remote builds in B9545: Diff 13239!

• karol edited the summary of this revision. (Show Details)May 31 2022, 7:09 AM

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

• karol added reviewers: ashoat, tomek, varun.

• karol added a parent revision: D4165: [services] Tests - Add create new backup.

• karol added a child revision: D4167: [DRAFT] [services] Tests - Add main.May 31 2022, 7:13 AM

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:24 AM

Harbormaster failed remote builds in B9552: Diff 13246!

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:28 AM

Harbormaster failed remote builds in B9557: Diff 13251!

I know that Ci failed but it's related to docker which I know is not working properly right now.

• karol added inline comments.May 31 2022, 7:45 AM

services/commtest/src/send_log.rs
13–14 ↗	(On Diff #13251)	Hacked it like this but I feel like this isn't a good solution.

I don't feel confident enough to review Rust :(

tomek requested changes to this revision.Jun 1 2022, 6:43 AM

tomek added inline comments.

services/commtest/src/send_log.rs
13–14 ↗	(On Diff #13251)	Maybe `String::from` or `.clone()` can be used?
16 ↗	(On Diff #13251)	Do we need a loop that does only one iteration?

This revision now requires changes to proceed.Jun 1 2022, 6:43 AM

address feedback

Harbormaster completed remote builds in B9636: Diff 13353.Jun 6 2022, 4:53 AM

• karol planned changes to this revision.Jun 7 2022, 6:46 AM

update

• karol added a child revision: D4227: [services] Tests - Add byte size crate.Jun 7 2022, 7:46 AM

Harbormaster completed remote builds in B9669: Diff 13396.Jun 7 2022, 7:49 AM

• karol removed a child revision: D4167: [DRAFT] [services] Tests - Add main.Jun 7 2022, 8:39 AM

tomek requested changes to this revision.Jun 8 2022, 5:55 AM

tomek added inline comments.

services/commtest/src/send_log.rs
32 ↗	(On Diff #13396)	Are these magic numbers meaningful?
45 ↗	(On Diff #13396)	How does this work? Does this send requests from a stream one by one?

This revision now requires changes to proceed.Jun 8 2022, 5:55 AM

• karol added inline comments.Jun 13 2022, 2:38 AM

services/commtest/src/send_log.rs
32 ↗	(On Diff #13396)	No, just random
45 ↗	(On Diff #13396)	so this takes the `outbound` that seems to be a generator and uses `yield` to send the following requests to the server. I think generators in rust are about to be similar to the ones in Python. So they lazily evaluate the results of a function. However, it seems that they're not fully implemented in rust yet: https://doc.rust-lang.org/beta/unstable-book/language-features/generators.html

• karol requested review of this revision.Jun 13 2022, 2:38 AM

tomek accepted this revision.Jun 13 2022, 2:58 AM

tomek added inline comments.

services/commtest/src/send_log.rs
45 ↗	(On Diff #13396)	And what about the return value? We have only a single value, `response`, but multiple requests (from a stream). Is it a return value of the last request? And about the stability of generators: according to the unstable book, the are really experimental. But we probably didn't need to set any flag to access the feature, so maybe the book is outdated? Could you check if they can be used safely? (it wouldn't be great if the API changed with the next rust version).