Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 31, 2017, 2:37 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
  include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 


Diff: https://reviews.apache.org/r/60932/diff/7/

Changes: https://reviews.apache.org/r/60932/diff/6-7/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181813
---


Fix it, then Ship it!





include/mesos/mesos.proto
Line 2918 (original), 2918 (patched)


Does this comment need updating now?



include/mesos/v1/mesos.proto
Line 2901 (original), 2901 (patched)


Does this comment need updating now?


- James Peach


On July 29, 2017, 11:58 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 29, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
>   include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 29, 2017, 4:58 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
  include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 


Diff: https://reviews.apache.org/r/60932/diff/6/

Changes: https://reviews.apache.org/r/60932/diff/5-6/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181683
---




include/mesos/mesos.proto
Lines 2920 (patched)


I think that this message describes a value, so I'd recommend calling it 
`Value` or even `OperationValue`.



include/mesos/v1/mesos.proto
Lines 2903 (patched)


Same comment about the name.


- James Peach


On July 26, 2017, 6:18 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 26, 2017, 6:18 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/5/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 25, 2017, 11:18 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
  include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 


Diff: https://reviews.apache.org/r/60932/diff/5/

Changes: https://reviews.apache.org/r/60932/diff/4-5/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181419
---




include/mesos/mesos.proto
Lines 2921-2922 (patched)


Please make the same changes to `include/mesos/v1/mesos.proto`.


- Qian Zhang


On July 26, 2017, 9:01 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 26, 2017, 9:01 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/4/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 25, 2017, 6:01 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
  include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 


Diff: https://reviews.apache.org/r/60932/diff/4/

Changes: https://reviews.apache.org/r/60932/diff/3-4/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181195
---


Ship it!




Ship It!

- Qian Zhang


On July 21, 2017, 7:57 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 21, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 20, 2017, 6:43 p.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?
> 
> Gilbert Song wrote:
> The same, I don't think an `optional` field hurts here and it makes it 
> more flexible. Let's chat tomorrow.
> 
> Qian Zhang wrote:
> Sure, let's chat on it. And can we have an expert on this area (BenM or 
> Jie) to provide some comments?
> 
> Jie Yu wrote:
> for this, i think we can make it required. we can always change it to 
> optional in the future.
> 
> Benjamin Mahler wrote:
> I think we should avoid using required fields, because we don't deal with 
> errors at the application level, right now the messages are dropped at the 
> parsing layers when required fields are missing.
> 
> We could update our parsing layers to use the "partial" parsing that 
> protobuf supplies 
> [[1]](https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io)
>  to ignore missing required fields, but we then have to update our 
> application level code to treat the field as optional anyway in terms of 
> validating it based on field presence. Also, I'm not sure we can assume the 
> use of partial parsing outside of mesos. So, it seems to me we might as well 
> try to stick with optional and perform application level validation of 
> missing fields (rather than let the parsing fail).
> 
> i.e.
> 
> `optional uint64 value = 2; // Required.`
> 
> Thoughts?
> 
> [1] 
> https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io
> 
> Jie Yu wrote:
> SGTM

Thank you guys! I should add some comments and the link to the kernel doc.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 20, 2017, 4:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Jie Yu]

> On July 21, 2017, 1:43 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?
> 
> Gilbert Song wrote:
> The same, I don't think an `optional` field hurts here and it makes it 
> more flexible. Let's chat tomorrow.
> 
> Qian Zhang wrote:
> Sure, let's chat on it. And can we have an expert on this area (BenM or 
> Jie) to provide some comments?
> 
> Jie Yu wrote:
> for this, i think we can make it required. we can always change it to 
> optional in the future.
> 
> Benjamin Mahler wrote:
> I think we should avoid using required fields, because we don't deal with 
> errors at the application level, right now the messages are dropped at the 
> parsing layers when required fields are missing.
> 
> We could update our parsing layers to use the "partial" parsing that 
> protobuf supplies 
> [[1]](https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io)
>  to ignore missing required fields, but we then have to update our 
> application level code to treat the field as optional anyway in terms of 
> validating it based on field presence. Also, I'm not sure we can assume the 
> use of partial parsing outside of mesos. So, it seems to me we might as well 
> try to stick with optional and perform application level validation of 
> missing fields (rather than let the parsing fail).
> 
> i.e.
> 
> `optional uint64 value = 2; // Required.`
> 
> Thoughts?
> 
> [1] 
> https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io

SGTM


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 20, 2017, 11:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Benjamin Mahler]

> On July 21, 2017, 1:43 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?
> 
> Gilbert Song wrote:
> The same, I don't think an `optional` field hurts here and it makes it 
> more flexible. Let's chat tomorrow.
> 
> Qian Zhang wrote:
> Sure, let's chat on it. And can we have an expert on this area (BenM or 
> Jie) to provide some comments?
> 
> Jie Yu wrote:
> for this, i think we can make it required. we can always change it to 
> optional in the future.

I think we should avoid using required fields, because we don't deal with 
errors at the application level, right now the messages are dropped at the 
parsing layers when required fields are missing.

We could update our parsing layers to use the "partial" parsing that protobuf 
supplies 
[[1]](https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io)
 to ignore missing required fields, but we then have to update our application 
level code to treat the field as optional anyway in terms of validating it 
based on field presence. Also, I'm not sure we can assume the use of partial 
parsing outside of mesos. So, it seems to me we might as well try to stick with 
optional and perform application level validation of missing fields (rather 
than let the parsing fail).

i.e.

`optional uint64 value = 2; // Required.`

Thoughts?

[1] 
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#heavy-io


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 20, 2017, 11:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Jie Yu]

> On July 21, 2017, 1:43 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?
> 
> Gilbert Song wrote:
> The same, I don't think an `optional` field hurts here and it makes it 
> more flexible. Let's chat tomorrow.
> 
> Qian Zhang wrote:
> Sure, let's chat on it. And can we have an expert on this area (BenM or 
> Jie) to provide some comments?

for this, i think we can make it required. we can always change it to optional 
in the future.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 20, 2017, 11:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Jie Yu]

> On July 20, 2017, 8:27 a.m., Qian Zhang wrote:
> > include/mesos/v1/mesos.proto
> > Lines 2815 (patched)
> > 
> >
> > Why do we need this? I see `Entry.op` is a required field rather than 
> > an optional field, so why do we need `UNKNOWN` for a required field?
> 
> Gilbert Song wrote:
> We will use `UNKNOWN` in all protobuf enums:
> https://issues.apache.org/jira/browse/MESOS-4997
> 
> Qian Zhang wrote:
> Yeah, I saw that. But that is for backwards compatibility, and the 
> solution mentioned in that ticket is `use optional enum fields and include an 
> UNKNOWN value as the first entry in the enum`, but in this patch, the enum 
> field `Entry.op` is required.
> 
> Qian Zhang wrote:
> In the latest patch, I see you have made `Entry.op` an optional field, 
> however as you mentioned in r60933:
> > Operations are required for those repeated fields in blkio protobuf
> 
> So I think it should still be a required field.
> 
> And for the backward compatibility issue mentioned in MESOS-4997, 
> basically I think it will only happen when server (e.g., Mesos master) tries 
> to parse the serialized protobuf string from client (e.g., framework), I do 
> not think this is gonna hanppen in your case. So I would suggest to make 
> `Entry.op` an required field and remove `UNKNOWN` from the `Operation` enum. 
> There are some existing examples that we can referece: the `Image.Type`, 
> `ContainerInfo.Type` and `DiscoveryInfo.Visibility`.
> 
> Gilbert Song wrote:
> After a second thought, I would still prefer `optional` since I don't 
> think an optional field hurts here and they should only be set in the blkio 
> subsystem. Also, considering we may upgrade to proto3 at some point, less 
> `required` field may make it easier, thoughts?

I'll stick with a UNKNOWN here. A custom resource estimator might use this 
protobuf so we do have a backwards compatibility issue here.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---


On July 20, 2017, 11:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

> On July 21, 2017, 9:43 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?
> 
> Gilbert Song wrote:
> The same, I don't think an `optional` field hurts here and it makes it 
> more flexible. Let's chat tomorrow.

Sure, let's chat on it. And can we have an expert on this area (BenM or Jie) to 
provide some comments?


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 21, 2017, 7:57 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 21, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 20, 2017, 1:27 a.m., Qian Zhang wrote:
> > include/mesos/v1/mesos.proto
> > Lines 2815 (patched)
> > 
> >
> > Why do we need this? I see `Entry.op` is a required field rather than 
> > an optional field, so why do we need `UNKNOWN` for a required field?
> 
> Gilbert Song wrote:
> We will use `UNKNOWN` in all protobuf enums:
> https://issues.apache.org/jira/browse/MESOS-4997
> 
> Qian Zhang wrote:
> Yeah, I saw that. But that is for backwards compatibility, and the 
> solution mentioned in that ticket is `use optional enum fields and include an 
> UNKNOWN value as the first entry in the enum`, but in this patch, the enum 
> field `Entry.op` is required.
> 
> Qian Zhang wrote:
> In the latest patch, I see you have made `Entry.op` an optional field, 
> however as you mentioned in r60933:
> > Operations are required for those repeated fields in blkio protobuf
> 
> So I think it should still be a required field.
> 
> And for the backward compatibility issue mentioned in MESOS-4997, 
> basically I think it will only happen when server (e.g., Mesos master) tries 
> to parse the serialized protobuf string from client (e.g., framework), I do 
> not think this is gonna hanppen in your case. So I would suggest to make 
> `Entry.op` an required field and remove `UNKNOWN` from the `Operation` enum. 
> There are some existing examples that we can referece: the `Image.Type`, 
> `ContainerInfo.Type` and `DiscoveryInfo.Visibility`.

After a second thought, I would still prefer `optional` since I don't think an 
optional field hurts here and they should only be set in the blkio subsystem. 
Also, considering we may upgrade to proto3 at some point, less `required` field 
may make it easier, thoughts?


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---


On July 20, 2017, 4:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 20, 2017, 6:43 p.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > 
> >
> > Why is `value` an optional field? I think there have to a value for any 
> > possible semantics, right?

The same, I don't think an `optional` field hurts here and it makes it more 
flexible. Let's chat tomorrow.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---


On July 20, 2017, 4:57 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

> On July 20, 2017, 4:27 p.m., Qian Zhang wrote:
> > include/mesos/v1/mesos.proto
> > Lines 2815 (patched)
> > 
> >
> > Why do we need this? I see `Entry.op` is a required field rather than 
> > an optional field, so why do we need `UNKNOWN` for a required field?
> 
> Gilbert Song wrote:
> We will use `UNKNOWN` in all protobuf enums:
> https://issues.apache.org/jira/browse/MESOS-4997
> 
> Qian Zhang wrote:
> Yeah, I saw that. But that is for backwards compatibility, and the 
> solution mentioned in that ticket is `use optional enum fields and include an 
> UNKNOWN value as the first entry in the enum`, but in this patch, the enum 
> field `Entry.op` is required.

In the latest patch, I see you have made `Entry.op` an optional field, however 
as you mentioned in r60933:
> Operations are required for those repeated fields in blkio protobuf

So I think it should still be a required field.

And for the backward compatibility issue mentioned in MESOS-4997, basically I 
think it will only happen when server (e.g., Mesos master) tries to parse the 
serialized protobuf string from client (e.g., framework), I do not think this 
is gonna hanppen in your case. So I would suggest to make `Entry.op` an 
required field and remove `UNKNOWN` from the `Operation` enum. There are some 
existing examples that we can referece: the `Image.Type`, `ContainerInfo.Type` 
and `DiscoveryInfo.Visibility`.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---


On July 21, 2017, 7:57 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 21, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181087
---




include/mesos/mesos.proto
Line 2843 (original), 2922 (patched)


Why is `value` an optional field? I think there have to a value for any 
possible semantics, right?


- Qian Zhang


On July 21, 2017, 7:57 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 21, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 20, 2017, 4:57 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
  include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 


Diff: https://reviews.apache.org/r/60932/diff/3/

Changes: https://reviews.apache.org/r/60932/diff/2-3/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

> On July 20, 2017, 4:27 p.m., Qian Zhang wrote:
> > include/mesos/v1/mesos.proto
> > Lines 2815 (patched)
> > 
> >
> > Why do we need this? I see `Entry.op` is a required field rather than 
> > an optional field, so why do we need `UNKNOWN` for a required field?
> 
> Gilbert Song wrote:
> We will use `UNKNOWN` in all protobuf enums:
> https://issues.apache.org/jira/browse/MESOS-4997

Yeah, I saw that. But that is for backwards compatibility, and the solution 
mentioned in that ticket is `use optional enum fields and include an UNKNOWN 
value as the first entry in the enum`, but in this patch, the enum field 
`Entry.op` is required.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---


On July 20, 2017, 8:17 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 8:17 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
>   include/mesos/v1/mesos.proto 5e92e5d86023ad6edd94303fbde964bf403abf02 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 20, 2017, 1:27 a.m., Qian Zhang wrote:
> > include/mesos/v1/mesos.proto
> > Lines 2815 (patched)
> > 
> >
> > Why do we need this? I see `Entry.op` is a required field rather than 
> > an optional field, so why do we need `UNKNOWN` for a required field?

We will use `UNKNOWN` in all protobuf enums:
https://issues.apache.org/jira/browse/MESOS-4997


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---


On July 19, 2017, 5:17 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 19, 2017, 5:17 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
>   include/mesos/v1/mesos.proto 5e92e5d86023ad6edd94303fbde964bf403abf02 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review181023
---




include/mesos/v1/mesos.proto
Lines 2815 (patched)


Why do we need this? I see `Entry.op` is a required field rather than an 
optional field, so why do we need `UNKNOWN` for a required field?


- Qian Zhang


On July 20, 2017, 8:17 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 20, 2017, 8:17 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
>   include/mesos/v1/mesos.proto 5e92e5d86023ad6edd94303fbde964bf403abf02 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/
---

(Updated July 19, 2017, 5:17 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


Changes
---

Addressed comments.


Bugs: MESOS-6162
https://issues.apache.org/jira/browse/MESOS-6162


Repository: mesos


Description (updated)
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
  include/mesos/v1/mesos.proto 5e92e5d86023ad6edd94303fbde964bf403abf02 


Diff: https://reviews.apache.org/r/60932/diff/2/

Changes: https://reviews.apache.org/r/60932/diff/1-2/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 19, 2017, 12:41 a.m., Qian Zhang wrote:
> > I think you also need to make the change to `include/mesos/v1/mesos.proto`.

good catch!


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180907
---


On July 18, 2017, 2:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 19, 2017, 12:02 p.m., Jie Yu wrote:
> > include/mesos/mesos.proto
> > Lines 1298 (patched)
> > 
> >
> > Can we re-use the existing `Device` in mesos.proto. I think we should 
> > make `path` optional.
> > ```
> > message Device {
> >   message Number {
> > required uint64 major;
> > required uint64 minor;
> >   }
> >   
> >   optional string path;
> >   optional Number number;
> > }
> > ```

make sense. I make this change on a separate patch.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180958
---


On July 18, 2017, 2:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2955 (patched)
> > 
> >
> > If we introduce a field like this, does that mean we will fill it in 
> > `BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?
> 
> Gilbert Song wrote:
> I would suggest let's remove this field and leave a TODO here, which will 
> be addressed when we introduce the `prepare` and `status` functions. What do 
> you think?
> 
> Qian Zhang wrote:
> So for `prepare()`, we are going to use `CgroupInfo.blkio` to do the 
> blkio control? If so, then I am OK with it. And for `status()`, what will be 
> the difference between it and `usage()`? Are they gonna return same statistic 
> about blkio?

I decide to remove all other fields except `statistics`. Let's introduce them 
later when necessary.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180794
---


On July 18, 2017, 2:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 19, 2017, 2:11 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 1525 (patched)
> > 
> >
> > s/blkio/blkio_statistics/ to keep consistent with the field 
> > `disk_statistics`.

+1


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180911
---


On July 18, 2017, 2:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180958
---




include/mesos/mesos.proto
Lines 1298 (patched)


Can we re-use the existing `Device` in mesos.proto. I think we should make 
`path` optional.
```
message Device {
  message Number {
required uint64 major;
required uint64 minor;
  }
  
  optional string path;
  optional Number number;
}
```


- Jie Yu


On July 18, 2017, 9:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 9:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180911
---




include/mesos/mesos.proto
Lines 1525 (patched)


s/blkio/blkio_statistics/ to keep consistent with the field 
`disk_statistics`.


- Qian Zhang


On July 19, 2017, 5:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 19, 2017, 5:39 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180907
---



I think you also need to make the change to `include/mesos/v1/mesos.proto`.

- Qian Zhang


On July 19, 2017, 5:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 19, 2017, 5:39 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

> On July 18, 2017, 9:31 p.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2886 (patched)
> > 
> >
> > Just curious, the reason that this is a `repeated` field is there are 5 
> > operations (total/read/write/sync/async)?
> 
> Gilbert Song wrote:
> Yea, per device there are 5 entries in the blkio.io_serviced file.

Got it, thanks Gilbert!


> On July 18, 2017, 9:31 p.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2955 (patched)
> > 
> >
> > If we introduce a field like this, does that mean we will fill it in 
> > `BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?
> 
> Gilbert Song wrote:
> I would suggest let's remove this field and leave a TODO here, which will 
> be addressed when we introduce the `prepare` and `status` functions. What do 
> you think?

So for `prepare()`, we are going to use `CgroupInfo.blkio` to do the blkio 
control? If so, then I am OK with it. And for `status()`, what will be the 
difference between it and `usage()`? Are they gonna return same statistic about 
blkio?


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180794
---


On July 19, 2017, 5:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 19, 2017, 5:39 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2886 (patched)
> > 
> >
> > Just curious, the reason that this is a `repeated` field is there are 5 
> > operations (total/read/write/sync/async)?

Yea, per device there are 5 entries in the blkio.io_serviced file.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180794
---


On July 17, 2017, 5:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 17, 2017, 5:44 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2950 (patched)
> > 
> >
> > Why do we need this msg? I do not see it is used anywhere.

This was left by Jason. Should be removed in favor of:

```
message Statistics {
  repeated CFQ.Statistics cfq = 1;
  repeated CFQ.Statistics cfq_recursive = 2;
  repeated Throttling.Statistics throttling = 3;
}
```

Thanks, Qian!


> On July 18, 2017, 6:31 a.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Lines 2955 (patched)
> > 
> >
> > If we introduce a field like this, does that mean we will fill it in 
> > `BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?

I would suggest let's remove this field and leave a TODO here, which will be 
addressed when we introduce the `prepare` and `status` functions. What do you 
think?


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180794
---


On July 17, 2017, 5:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 17, 2017, 5:44 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60932/#review180794
---




include/mesos/mesos.proto
Lines 2886 (patched)


Just curious, the reason that this is a `repeated` field is there are 5 
operations (total/read/write/sync/async)?



include/mesos/mesos.proto
Lines 2950 (patched)


Why do we need this msg? I do not see it is used anywhere.



include/mesos/mesos.proto
Lines 2955 (patched)


If we introduce a field like this, does that mean we will fill it in 
`BlkioSubsystem::status()` since `CgroupInfo` is part of `ContainerStatus`?


- Qian Zhang


On July 18, 2017, 8:44 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 18, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 
> Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf scheme for blkio subsystem in CgroupInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>