Details

Reviewers

• jon
• max
tomek
varun

Commits

rCOMM9c57479adedf: [services] Rust Integration - Backup - Rust - Fix wrong order of logs in the…

Summary

Depends on D5071

Since some logs are pulled later and some sooner, we have to identify them by IDs instead of the order in which they come from the server. For example, if you have the logs of size 3MB, 3MB, 5KB, or 3MB, first you'll probably receive this of 5KB and only then the rest.

Test Plan

integration tests should pass for backup.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Sep 6 2022, 7:22 AM

Herald added a reviewer: • jon. · View Herald TranscriptSep 6 2022, 7:22 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B11929: Diff 16375.Sep 6 2022, 7:33 AM

• karol requested review of this revision.Sep 6 2022, 7:33 AM

• karol edited the summary of this revision. (Show Details)Sep 6 2022, 7:35 AM

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

• karol added reviewers: • max, tomek, varun.

• karol added a parent revision: D5071: [services] Rust Integration - c++ - Implement worker in write reactor.

• karol added a child revision: D5073: [services] Rust Integration - Backup - c++ - Use uuid instead of current timestamp when generating log id.Sep 6 2022, 7:37 AM

• karol mentioned this in D5074: [services] Rust Integration - Backup - c++ - Fix wrong order of attachment holders in tests.Sep 6 2022, 7:40 AM

integration tests should pass for backup.

Is it the case that integration tests are failing now? If this is the case, which diff broke them? We have to make sure that tests pass after every diff.

services/commtest/tests/backup/backup_utils.rs
85–95 ↗	(On Diff #16375)	We also need to check if there are no items that are included in `result_log_map` but aren't present in `expected_log_map`.

This revision now requires changes to proceed.Sep 9 2022, 5:44 AM

• karol mentioned this in D5098: [services] Rust Integration - Backup - Rust - Fix backup utils.Sep 9 2022, 7:09 AM

address feedback

In D5072#148855, @tomek wrote:

Is it the case that integration tests are failing now? If this is the case, which diff broke them? We have to make sure that tests pass after every diff.

I'm not sure I follow here, sorry. Integration tests pass. What is the problem?

services/commtest/tests/backup/backup_utils.rs
85–95 ↗	(On Diff #16375)	Sure. But I think it would suffice to check if there are no duplicated log ids. In this case, we perform `insert` on both collections the same amount of times, so they both have the same size. If so, when we iterate over only one of them and for each item check if the corresponding item exists in the other collection, then we're sure that they have the same set of keys. Correct me if I'm wrong.

Harbormaster completed remote builds in B12077: Diff 16577.Sep 12 2022, 2:15 AM

In D5072#149159, @karol wrote:

In D5072#148855, @tomek wrote:

Is it the case that integration tests are failing now? If this is the case, which diff broke them? We have to make sure that tests pass after every diff.

I'm not sure I follow here, sorry. Integration tests pass. What is the problem?

In that case the problem is that your test plan

integration tests should pass for backup.

Doesn't check anything if the tests were passing before and after. To make this test plan really beneficial, we need to introduce a test that wouldn't pass before this diff but passes after it. Otherwise, how would we know if this diff improves anything?

This revision is now accepted and ready to land.Sep 12 2022, 2:56 AM

Ok, I see your point. It feels like you think I just never described what was wrong with those tests. I kind of described what was the problem in the description. Before this revision, the integration tests tended to not work because of the reasons I described. This revision eliminated this problem.

remove redundant code

Harbormaster completed remote builds in B12109: Diff 16616.Sep 13 2022, 1:17 AM

In D5072#149179, @karol wrote:

Ok, I see your point. It feels like you think I just never described what was wrong with those tests. I kind of described what was the problem in the description. Before this revision, the integration tests tended to not work because of the reasons I described. This revision eliminated this problem.

That makes sense, thanks!

rebase

Harbormaster completed remote builds in B12242: Diff 16794.Sep 19 2022, 1:10 AM

Closed by commit rCOMM9c57479adedf: [services] Rust Integration - Backup - Rust - Fix wrong order of logs in the… (authored by • karol). · Explain WhySep 19 2022, 1:11 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM9c57479adedf: [services] Rust Integration - Backup - Rust - Fix wrong order of logs in the….

• karol mentioned this in rCOMM246f985561db: [services] Rust Integration - Backup - c++ - Fix wrong order of attachment….