You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Dmitry Zhuk <dz...@twopensource.com> on 2017/10/11 17:34:44 UTC
Review Request 62900: Enabled protobuf arenas code generation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62900/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6971
https://issues.apache.org/jira/browse/MESOS-6971
Repository: mesos
Description
-------
This adds `option cc_enable_arenas = true;` to relevant proto files.
Diffs
-----
include/mesos/authentication/authentication.proto 3869d6b117ead633dbc6ae23aa5059d13c9ff70d
include/mesos/master/master.proto 79be497984cdecf91850264734243af29c060d85
include/mesos/mesos.proto 830985a3265b7c104d8fdc50749c395d98f5f3c8
include/mesos/scheduler/scheduler.proto 0528a7ea2df0aa4bf6d4405274943e9858998812
src/messages/log.proto ca740bd011147f8f48cc5dfcacefa5fdbd95ccd0
src/messages/messages.proto afca6d161c2c2049eeffe4189d86b460b345dd51
Diff: https://reviews.apache.org/r/62900/diff/1/
Testing
-------
make
Thanks,
Dmitry Zhuk
Re: Review Request 62900: Enabled protobuf arenas code generation.
Posted by Dmitry Zhuk <dz...@twopensource.com>.
> On Oct. 12, 2017, 2:19 a.m., Benjamin Mahler wrote:
> > I assume you're only touching the files that use protobuf `install` handlers because there's a build speed cost?
I just changed minimal set of files required to make it compile with the changes in #62901. I didn't notice any impact on build speed, but I use quite powerful build machine. Apart from possible impact on build time, there's a minimal runtime penalty for having arenas (protobuf needs to compare if objects come from the same arena in some cases) + more binary code.
- Dmitry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62900/#review187726
-----------------------------------------------------------
On Oct. 11, 2017, 5:34 p.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62900/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2017, 5:34 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6971
> https://issues.apache.org/jira/browse/MESOS-6971
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This adds `option cc_enable_arenas = true;` to relevant proto files.
>
>
> Diffs
> -----
>
> include/mesos/authentication/authentication.proto 3869d6b117ead633dbc6ae23aa5059d13c9ff70d
> include/mesos/master/master.proto 79be497984cdecf91850264734243af29c060d85
> include/mesos/mesos.proto 830985a3265b7c104d8fdc50749c395d98f5f3c8
> include/mesos/scheduler/scheduler.proto 0528a7ea2df0aa4bf6d4405274943e9858998812
> src/messages/log.proto ca740bd011147f8f48cc5dfcacefa5fdbd95ccd0
> src/messages/messages.proto afca6d161c2c2049eeffe4189d86b460b345dd51
>
>
> Diff: https://reviews.apache.org/r/62900/diff/1/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Dmitry Zhuk
>
>
Re: Review Request 62900: Enabled protobuf arenas code generation.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62900/#review187726
-----------------------------------------------------------
Fix it, then Ship it!
I assume you're only touching the files that use protobuf `install` handlers because there's a build speed cost?
src/messages/log.proto
Lines 19 (patched)
<https://reviews.apache.org/r/62900/#comment264778>
Shall we have the option consistently below the package?
src/messages/messages.proto
Lines 19 (patched)
<https://reviews.apache.org/r/62900/#comment264779>
Shall we have the option consistently below the package?
- Benjamin Mahler
On Oct. 11, 2017, 5:34 p.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62900/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2017, 5:34 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6971
> https://issues.apache.org/jira/browse/MESOS-6971
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This adds `option cc_enable_arenas = true;` to relevant proto files.
>
>
> Diffs
> -----
>
> include/mesos/authentication/authentication.proto 3869d6b117ead633dbc6ae23aa5059d13c9ff70d
> include/mesos/master/master.proto 79be497984cdecf91850264734243af29c060d85
> include/mesos/mesos.proto 830985a3265b7c104d8fdc50749c395d98f5f3c8
> include/mesos/scheduler/scheduler.proto 0528a7ea2df0aa4bf6d4405274943e9858998812
> src/messages/log.proto ca740bd011147f8f48cc5dfcacefa5fdbd95ccd0
> src/messages/messages.proto afca6d161c2c2049eeffe4189d86b460b345dd51
>
>
> Diff: https://reviews.apache.org/r/62900/diff/1/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Dmitry Zhuk
>
>
Re: Review Request 62900: Enabled protobuf arenas code generation.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62900/#review195693
-----------------------------------------------------------
Do we need corresponding changes on v1/***.proto?
- Gilbert Song
On Oct. 11, 2017, 10:34 a.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62900/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2017, 10:34 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6971
> https://issues.apache.org/jira/browse/MESOS-6971
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This adds `option cc_enable_arenas = true;` to relevant proto files.
>
>
> Diffs
> -----
>
> include/mesos/authentication/authentication.proto 3869d6b117ead633dbc6ae23aa5059d13c9ff70d
> include/mesos/master/master.proto 79be497984cdecf91850264734243af29c060d85
> include/mesos/mesos.proto 830985a3265b7c104d8fdc50749c395d98f5f3c8
> include/mesos/scheduler/scheduler.proto 0528a7ea2df0aa4bf6d4405274943e9858998812
> src/messages/log.proto ca740bd011147f8f48cc5dfcacefa5fdbd95ccd0
> src/messages/messages.proto afca6d161c2c2049eeffe4189d86b460b345dd51
>
>
> Diff: https://reviews.apache.org/r/62900/diff/1/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Dmitry Zhuk
>
>