Details
- Reviewers
olasd zack - Group Reviewers
Reviewers - Maniphest Tasks
- T512: Make archiver have a more symmetrical behavior treating storages as potential sources & destinations at the same time
- Commits
- rDSTOC591e886ada07: Update the archiver specification
R65:591e886ada07: Update the archiver specification
rDARC6d793d4027ec: Update the archiver specification
rDSTO591e886ada07: Update the archiver specification
Diff Detail
- Repository
- rDSTO Storage manager
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
| docs/archiver-blueprint.md | ||
|---|---|---|
| 15 | "Peer-to-peer" (or P2P) is the standard spelling of this notion | |
| 15–24 | A more substantial comment here is that the recent changes in how the archiver works didn't really turn it into a P2P system. Most notably we still rely on a centralized director that: a) spots that more copies are needed, and b) decides what-to-copy-where. So I propose to call this section "Peer-to-peer topology". And maybe add a paragraph to state that, whereas peers are in general considered to be equal, coordination is currently centralized. | |
| 34–40 | Here is a plug to reiterate, if needed, that the source/destination decision is not up to individual nodes. | |
| 97–103 | why is the discussion of mtime gone from here? | |
| 114 | same as above: why is this gone? | |
| 119 | I don't understand what "but elapsed time" means here, please clarify/rewrite. | |
| 128–134 | nitpick: there are capital letters at the beginning of bullet points, whereas they aren't there all over the document | |
| 130 | nitpick: writing this "for each (content, source, destination)" would make it less odd | |
| 133 | typo: "transfert" -> "transfer" | |
| 168–169 | typo: "api" -> "API" | |
| 177–178 | maybe add a comment here that these are sample/initial archive names | |
| 196 | The schema of the JSON here should be described, in a way that allows validation, ideally using json-schema. | |
Corrections of the specs and add the json schema description
| docs/archiver-blueprint.md | ||
|---|---|---|
| 97–103 | The new specification does not check /mtime/ in the director. It only count the number of effectively present copies to select contents that need or not more copies. This makes the batches less accurate about contents that really needs archival, but they are generated quicker, and as this check was done anyway in the worker, we can save a db request for each content. | |
All good! Just a couple of new nits remaining.
| docs/archiver-blueprint.md | ||
|---|---|---|
| 97–103 | OK, fair enough. Thanks for clarifying. | |
| 204–223 | Several separator "," are missing in this JSON. | |
| 214–215 | A description of valid values is missing here, adding the following line here should do: "enum": ["missing", "ongoing", "present", "corrupted"] (but please check the JSON Schema spec because I'm not 100% sure) | |
Correct json format and move integrity check from copier to worker (previous position was a mistake)
| docs/archiver-blueprint.md | ||
|---|---|---|
| 214–215 | "enum" seems valid. Thanks ! | |