Depends on D4636
Calculating and sending real sha512 hash instead of some mocked bytes when creating a new backup and sending logs
Details
Details
term 1
cd services yarn run-blob-service-in-sandbox
term 2
cd services yarn run-backup-service-in-sandbox
term 3
cd services yarn run-integration-tests backup
check in the database that you have real hashes instead of something like ABCD
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- performance-tests
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
It would be better to have an abstraction over hash algorithm so that when we decide to change it, we don't need to modify its usages.
Adding @ashoat because there are new dependencies.
| services/commtest/tests/backup/send_log.rs | ||
|---|---|---|
| 42–55 ↗ | (On Diff #15060) | I don't understand this logic. We generate some data and compute its hash. Then we generate data again and send it. If we generate data twice, it should be different every time. Shouldn't we compute hash and send the same data? |
Comment Actions
It would be better to have an abstraction over hash algorithm so that when we decide to change it, we don't need to modify its usages.
Fair point, we can exclude it to an external place.
| services/commtest/tests/backup/send_log.rs | ||
|---|---|---|
| 42–55 ↗ | (On Diff #15060) | I admit this logic could be better but the problem is, we're first required to provide a hash and only then the chunks. I didn't want to keep a lot of data in the memory so I figured it's better to generate the data twice. The function generate_nbytes generates the same output when called multiple times with the same input, so we're going to send the same data as we calculated the hash for. My guess is that we've already discussed this and decided to first send the hash and then the data for some reason. If you feel like rediscussing it, I'm open and we can get back to this and discuss switching the order. If you feel like there should be more context for this, I can spend some time and try to figure out the reasons behind the chosen approach - just let me know. |
Comment Actions
New dependencies seem reasonable; deferring to other reviewers on the rest of the review
| services/commtest/tests/backup/send_log.rs | ||
|---|---|---|
| 42–55 ↗ | (On Diff #15060) | Not sure if this is relevant, but if this leads to increased round-trips between server/client (eg. what we were trying to address in ENG-1052) then I would support revisiting this discussion |
| services/commtest/tests/backup/send_log.rs | ||
|---|---|---|
| 42–55 ↗ | (On Diff #15060) | Sending the hash first and then the data is good enough at this point. The issue I have has to do with just the test. It isn't obvious that generate_nbytes
I think this can change at some point. If we are sure that it will not change, then we should change the name to e.g. generate_stable_nbytes - to indicate that repeated calls will return the same value. But the main reason behind introducing this logic is
which doesn't make too much sense to me. Currently, every call to this method would result in generating a couple of kB of data - it's ok to use that much of memory. And even if we decide to generate more, even hundreds of MB should be fine. |
Comment Actions
renamse function to stable
| services/commtest/tests/backup/send_log.rs | ||
|---|---|---|
| 42–55 ↗ | (On Diff #15060) | I just didn't want to go unlimited with this. Ok, I'm going to update the name to generate_stable_nbytes. |