----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64868/#review194631 -----------------------------------------------------------
docs/csi.md Lines 21 (patched) <https://reviews.apache.org/r/64868/#comment273500> Nit: s/well defined/well-defined/ docs/csi.md Lines 25 (patched) <https://reviews.apache.org/r/64868/#comment273501> Is "currently" appropriate here? Do we expect this to change? IIUC, this feature is distinct from CSI support and still remains in the codebase? docs/csi.md Lines 28 (patched) <https://reviews.apache.org/r/64868/#comment273502> s/reservation, fair/reservation, and fair/ docs/csi.md Lines 30 (patched) <https://reviews.apache.org/r/64868/#comment273503> Nit: s/Volume Driver/volume driver/ docs/csi.md Lines 39 (patched) <https://reviews.apache.org/r/64868/#comment273504> s/from/from the/ docs/csi.md Lines 41 (patched) <https://reviews.apache.org/r/64868/#comment273505> Suggestion: s/to have storage vendors write/to allow storage vendors to write/ docs/csi.md Lines 45 (patched) <https://reviews.apache.org/r/64868/#comment273506> s/big/larger/ I would also recommend: s/very// docs/csi.md Lines 46 (patched) <https://reviews.apache.org/r/64868/#comment273507> Suggestion: s/the users/users/ docs/csi.md Lines 101 (patched) <https://reviews.apache.org/r/64868/#comment273508> Nit: s/plugin specific/plugin-specific/ docs/csi.md Lines 109 (patched) <https://reviews.apache.org/r/64868/#comment273512> I'm curious: is this accurate as written, or would "must not be set by frameworks" be more appropriate? Similar question regarding the comment for `metadata`. docs/csi.md Lines 130 (patched) <https://reviews.apache.org/r/64868/#comment273515> Suggestion: s/if the `RAW` disk resource is backed by a CSI Volume or not/whether or not the `RAW` disk resource is backed by a CSI Volume/ docs/csi.md Lines 146 (patched) <https://reviews.apache.org/r/64868/#comment273517> s/using/using the/ docs/csi.md Lines 163 (patched) <https://reviews.apache.org/r/64868/#comment273520> For consistency with the scheduler API docs, let's do s/framework API/scheduler API/ here and below. docs/csi.md Lines 192 (patched) <https://reviews.apache.org/r/64868/#comment273523> Will we duplicate these offer operation docs in the scheduler API documentation? To avoid duplication, we could instead add these instructions to the scheduler API docs, and simply have a link here with a couple sentences explaining what these operations are used for. docs/csi.md Lines 285 (patched) <https://reviews.apache.org/r/64868/#comment273524> either s/disks/disk/ or s/disks/disk's/ docs/csi.md Lines 289-290 (patched) <https://reviews.apache.org/r/64868/#comment273527> Is it better to just say "by looking at the resources in subsequent offers"? Are there other sources of information schedulers should use? docs/csi.md Lines 291-296 (patched) <https://reviews.apache.org/r/64868/#comment273526> Should we also touch on the issue of roles w.r.t. receiving the converted resource in an offer? Are volumes created by the new operations similar to persistent volumes in that they can only be performed on reserved resources? docs/csi.md Lines 298 (patched) <https://reviews.apache.org/r/64868/#comment273525> I would recommend simply ommitting this section until we implement it. docs/csi.md Lines 328 (patched) <https://reviews.apache.org/r/64868/#comment273528> Nit: s/vendor specific/vendor-specific/ Here and in the comments below. docs/csi.md Lines 330 (patched) <https://reviews.apache.org/r/64868/#comment273529> Nit: s/low level/low-level/ Here and just below the proto snippet. docs/csi.md Lines 360 (patched) <https://reviews.apache.org/r/64868/#comment273530> s/system specific/system-specific/ docs/csi.md Lines 386 (patched) <https://reviews.apache.org/r/64868/#comment273533> s/will/will be/ docs/csi.md Lines 387 (patched) <https://reviews.apache.org/r/64868/#comment273534> s/call/call the/ docs/csi.md Lines 408 (patched) <https://reviews.apache.org/r/64868/#comment273536> s/under a/under/ docs/csi.md Lines 408-409 (patched) <https://reviews.apache.org/r/64868/#comment273537> Suggestion: s/a free-form string-string mapping/arbitrary key-value pairs/ docs/csi.md Lines 432 (patched) <https://reviews.apache.org/r/64868/#comment273538> s/--uri/uri/ Here and below. docs/csi.md Lines 433-435 (patched) <https://reviews.apache.org/r/64868/#comment273540> Suggestion: "If the poll interval has elapsed since the last fetch, then the URI is re-fetched; otherwise, a cached `ProfileInfo` is returned. If not specified, the URI is only fetched once." docs/csi.md Lines 442 (patched) <https://reviews.apache.org/r/64868/#comment273541> s/follow modules doc and add/follow the modules documentation: add/ docs/csi.md Lines 477-478 (patched) <https://reviews.apache.org/r/64868/#comment273542> s/has/have/ docs/csi.md Lines 485 (patched) <https://reviews.apache.org/r/64868/#comment273543> s/with/with it/ docs/csi.md Lines 509 (patched) <https://reviews.apache.org/r/64868/#comment273544> s/administrator/administrators/ docs/csi.md Lines 515 (patched) <https://reviews.apache.org/r/64868/#comment273545> s/using gRPC/using the gRPC/ docs/csi.md Lines 522 (patched) <https://reviews.apache.org/r/64868/#comment273546> s/only/only the/ docs/csi.md Lines 523 (patched) <https://reviews.apache.org/r/64868/#comment273547> s/thus/and thus/ docs/csi.md Lines 525 (patched) <https://reviews.apache.org/r/64868/#comment273548> Is there a JIRA we can link here? docs/csi.md Lines 531 (patched) <https://reviews.apache.org/r/64868/#comment273549> s/turn/turned/ docs/csi.md Lines 537-539 (patched) <https://reviews.apache.org/r/64868/#comment273550> Are these strictly necessary for SLRP support? docs/csi.md Lines 625 (patched) <https://reviews.apache.org/r/64868/#comment273552> I'm guessing the CSI plugin container will have all isolation mechanisms configured on the agent applied to it? Might be worth noting here. docs/csi.md Lines 699 (patched) <https://reviews.apache.org/r/64868/#comment273553> I'm guessing the directory may be empty in this case? Might be worth noting. docs/csi.md Lines 700 (patched) <https://reviews.apache.org/r/64868/#comment273554> s/Call/call/ docs/csi.md Lines 702-721 (patched) <https://reviews.apache.org/r/64868/#comment273557> I might just move this into the operator API docs since you have a link to those here. The curl example makes the form of the call explicit. Ditto for the other calls below. docs/csi.md Lines 764-766 (patched) <https://reviews.apache.org/r/64868/#comment273560> s/storage.containers/storage.plugin.containers/ Maybe include a bold **NOTE** here? This is a significant caveat. docs/csi.md Lines 828-841 (patched) <https://reviews.apache.org/r/64868/#comment273561> I might recommend a simple description of the ACL subject/object, rather than the inlined proto message. docs/csi.md Lines 843-844 (patched) <https://reviews.apache.org/r/64868/#comment273562> s/`SOME` is not yet supported/Fine-grained authorization of specific resource provider objects is not yet supported./ - Greg Mann On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64868/ > ----------------------------------------------------------- > > (Updated Dec. 29, 2017, 4:53 a.m.) > > > Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, > Greg Mann, Joseph Wu, and Jan Schlicht. > > > Repository: mesos > > > Description > ------- > > Added initial doc about CSI support in Mesos. > > > Diffs > ----- > > docs/csi.md PRE-CREATION > > > Diff: https://reviews.apache.org/r/64868/diff/2/ > > > Testing > ------- > > The rendering can be checked here: > https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md > > > Thanks, > > Jie Yu > >
