mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.
Date Fri, 21 Jul 2017 23:48:11 GMT


> On July 20, 2017, 6:43 p.m., Qian Zhang wrote:
> > include/mesos/mesos.proto
> > Line 2843 (original), 2922 (patched)
> > <https://reviews.apache.org/r/60932/diff/2-3/?file=1780157#file1780157line2922>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message