Details
-
Task
-
Resolution: Fixed
-
Major
-
master
-
0
Description
James Lee brought up a number of issues on my revert of the revert of the magma cbdatarecovery changes:
| File | Line | Comment |
|---|---|---|
| magma/reader.go | 230 | This feels the same as collections.DecodeKey, we could just rename that, and return a uint64 |
| transferable/recovery.go | 72 | IMO this belongs in transferable/utils.go with the other plan getters. |
| transferable/utils_recovery_test.go | This filename doesn't match what's being tested, I believe it should be recovery_test.go if we're moving this stuff out of utils.go. Perhaps it's done this way for build flags, but seems odd to have test cases which don't outwardly appear to be magma related, to be cordoned off like this. Moving it is also causing code-churn, meaning it's unclear whether this function has just been moved, or moved and changed. | |
| recovery/couchstore_test.go | 30 | same applies is quite a few places, but we should be using assert and require. |
| recovery/magma.go | 22 | Missing documentation. |
| recovery/source.go | 86 | Not a fan of "finalErr", we should use errdefs.MultiError and capture everything. |
| recovery/magma.go | 22 | Missing documentation. |
| recovery/couchstore_test.go | 30 | same applies is quite a few places, but we should be using assert and require. |
As these were not caught in the original review I have deferred them from reverting the revert into this ticket
Attachments
| For Gerrit Dashboard: MB-59195 | ||||||
|---|---|---|---|---|---|---|
| # | Subject | Branch | Project | Status | CR | V |
| 201584,3 | MB-59195 Magma recovery refactors | master | backup | Status: MERGED | +2 | +1 |
| 201585,5 | MB-59195 Use require rather than t.Fatal in tests | master | backup | Status: MERGED | +2 | +1 |
| 201586,5 | MB-59195 Add documentation to magma store | master | backup | Status: MERGED | +2 | +1 |