You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2017/12/02 01:10:11 UTC
Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/
-----------------------------------------------------------
Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
Bugs: MESOS-8294
https://issues.apache.org/jira/browse/MESOS-8294
Repository: mesos
Description
-------
Added a flag conversion protobuf message 'ImageGcConfig'.
Diffs
-----
src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
Diff: https://reviews.apache.org/r/64265/diff/1/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review193027
-----------------------------------------------------------
Ship it!
Ship It!
- Zhitao Li
On Dec. 6, 2017, 7:07 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2017, 7:07 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/
-----------------------------------------------------------
(Updated Dec. 6, 2017, 11:07 a.m.)
Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
Bugs: MESOS-8294
https://issues.apache.org/jira/browse/MESOS-8294
Repository: mesos
Description
-------
Added a flag conversion protobuf message 'ImageGcConfig'.
Diffs (updated)
-----
src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
Diff: https://reviews.apache.org/r/64265/diff/2/
Changes: https://reviews.apache.org/r/64265/diff/1-2/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 3, 2017, 2:14 p.m., Zhitao Li wrote:
> > src/messages/flags.proto
> > Lines 112-113 (patched)
> > <https://reviews.apache.org/r/64265/diff/1/?file=1906399#file1906399line112>
> >
> > `required` fields are generally harder to handler during upgrade once we introduce them. Can we find sane way to declare them `optional`? (Mayor be use comment to indicate what non-zero actual value is used when not set?)
Chatted with @Zhitao offline. Should be ok to change `required` field to `optional` if we need to deprecation these fields in an upgrade.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192650
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192650
-----------------------------------------------------------
src/messages/flags.proto
Lines 112-113 (patched)
<https://reviews.apache.org/r/64265/#comment270909>
`required` fields are generally harder to handler during upgrade once we introduce them. Can we find sane way to declare them `optional`? (Mayor be use comment to indicate what non-zero actual value is used when not set?)
- Zhitao Li
On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 1:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 1, 2017, 11:51 p.m., Qian Zhang wrote:
> > src/messages/flags.proto
> > Lines 112-114 (patched)
> > <https://reviews.apache.org/r/64265/diff/1/?file=1906399#file1906399line112>
> >
> > Can we add comments to describe each of the fields?
will do.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192619
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192619
-----------------------------------------------------------
Fix it, then Ship it!
src/messages/flags.proto
Lines 112-114 (patched)
<https://reviews.apache.org/r/64265/#comment270883>
Can we add comments to describe each of the fields?
- Qian Zhang
On Dec. 2, 2017, 9:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 9:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192611
-----------------------------------------------------------
I will add some comments for each protobuf field in the message.
- Gilbert Song
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 2, 2017, 12:49 a.m., Qian Zhang wrote:
> > src/messages/flags.proto
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/64265/diff/1/?file=1906399#file1906399line113>
> >
> > DurationInfo seems a too small time unit (nanosecond) which may not convenient for operator to specify, can we use a larger unit instead (like second)?
>
> Gilbert Song wrote:
> was thinking the same thing. here is another option:
>
> `required double image_disk_watch_interval_seconds = 2;`
>
> Zhitao Li wrote:
> -1 to `double` + seconds: it produces non-precise machine value (i.e, we cannot precisely define 1ms with a floating double).
Chatted with @Vinod and @Zhitao offline. We have 3 options:
1. Introduce a new protobuf `DurationValue`: (tech debt)
```
message DurationValue {
enum Unit {
SECONDS = 0;
MINUTES = 1;
....
}
requried Unit unit = 1;
required int64 value = 2;
}
```
But this would related to many other duration definition. They belong to tech debts that should be addressed together.
2. Define it as a `string` and leverage the `Duration::Paser()` to parse it. (not compatible with some other language library standard, eg., GO language library).
3. Use `required double image_disk_watch_interval_seconds = 2;` (not accurate)
After a second thought, I decide to still use `nanoseconds` as it is accurate enough. we can deprecate it once we have opeiton #1 in the future.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192620
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 2, 2017, 12:49 a.m., Qian Zhang wrote:
> > src/messages/flags.proto
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/64265/diff/1/?file=1906399#file1906399line113>
> >
> > DurationInfo seems a too small time unit (nanosecond) which may not convenient for operator to specify, can we use a larger unit instead (like second)?
was thinking the same thing. here is another option:
`required double image_disk_watch_interval_seconds = 2;`
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192620
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Zhitao Li <zh...@gmail.com>.
> On Dec. 2, 2017, 8:49 a.m., Qian Zhang wrote:
> > src/messages/flags.proto
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/64265/diff/1/?file=1906399#file1906399line113>
> >
> > DurationInfo seems a too small time unit (nanosecond) which may not convenient for operator to specify, can we use a larger unit instead (like second)?
>
> Gilbert Song wrote:
> was thinking the same thing. here is another option:
>
> `required double image_disk_watch_interval_seconds = 2;`
-1 to `double` + seconds: it produces non-precise machine value (i.e, we cannot precisely define 1ms with a floating double).
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192620
-----------------------------------------------------------
On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 1:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64265: Added a flag conversion protobuf message
'ImageGcConfig'.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64265/#review192620
-----------------------------------------------------------
src/messages/flags.proto
Lines 113 (patched)
<https://reviews.apache.org/r/64265/#comment270885>
DurationInfo seems a too small time unit (nanosecond) which may not convenient for operator to specify, can we use a larger unit instead (like second)?
- Qian Zhang
On Dec. 2, 2017, 9:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64265/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 9:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a flag conversion protobuf message 'ImageGcConfig'.
>
>
> Diffs
> -----
>
> src/messages/flags.proto 7ae9ef82cf9e918cac1eadc9f3ec0534ad4922b2
>
>
> Diff: https://reviews.apache.org/r/64265/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>