You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/06/12 02:06:18 UTC
Review Request 59988: Added a new protobuf message `MapMessage` for
protobuf tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Zhitao Li.
Repository: mesos
Description
-------
Added a new protobuf message `MapMessage` for protobuf tests.
Diffs
-----
3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
Diff: https://reviews.apache.org/r/59988/diff/1/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On June 12, 2017, 5:41 p.m., Zhitao Li wrote:
> > It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
> >
> > Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
>
> Qian Zhang wrote:
> Yes, the `.proto` change is the only real code changed, I have updated the description to mention that. And for the test, please refer to https://reviews.apache.org/r/59989/.
>
> Benjamin Bannier wrote:
> Could you have a look and try to find out how hard it would be to generate the `*.pb.*` files during the build instead of checking them in, https://issues.apache.org/jira/browse/MESOS-6115?
>
> Qian Zhang wrote:
> In MESOS-6115, I see you mentioned:
> > We should try to remove them from the source tree; in fact protobuf_tests.cpp already documents ways this could be achieved.
>
> I am not sure what you meant for `protobuf_tests.cpp already documents ways this could be achieved.`, did you mean the comments here: https://github.com/apache/mesos/blob/1.3.0/3rdparty/stout/tests/protobuf_tests.cpp#L88:L104 which seems the way to generate protobuf message dynamically in the runtime rather than during the build.
I am not sure what I was referring to there, so I removed that line from the ticket. It seems just adding a rule to generate these output files and adding them to the test binary like we already do would do the job.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
On June 13, 2017, 10:41 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 10:41 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On June 12, 2017, 5:41 p.m., Zhitao Li wrote:
> > It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
> >
> > Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
>
> Qian Zhang wrote:
> Yes, the `.proto` change is the only real code changed, I have updated the description to mention that. And for the test, please refer to https://reviews.apache.org/r/59989/.
Could you have a look and try to find out how hard it would be to generate the `*.pb.*` files during the build instead of checking them in, https://issues.apache.org/jira/browse/MESOS-6115?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
On June 13, 2017, 10:41 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 10:41 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
> On June 12, 2017, 11:41 p.m., Zhitao Li wrote:
> > It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
> >
> > Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
Yes, the `.proto` change is the only real code changed, I have updated the description to mention that. And for the test, please refer to https://reviews.apache.org/r/59989/.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
On June 13, 2017, 4:41 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 4:41 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
> On June 12, 2017, 11:41 p.m., Zhitao Li wrote:
> > It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
> >
> > Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
>
> Qian Zhang wrote:
> Yes, the `.proto` change is the only real code changed, I have updated the description to mention that. And for the test, please refer to https://reviews.apache.org/r/59989/.
>
> Benjamin Bannier wrote:
> Could you have a look and try to find out how hard it would be to generate the `*.pb.*` files during the build instead of checking them in, https://issues.apache.org/jira/browse/MESOS-6115?
>
> Qian Zhang wrote:
> In MESOS-6115, I see you mentioned:
> > We should try to remove them from the source tree; in fact protobuf_tests.cpp already documents ways this could be achieved.
>
> I am not sure what you meant for `protobuf_tests.cpp already documents ways this could be achieved.`, did you mean the comments here: https://github.com/apache/mesos/blob/1.3.0/3rdparty/stout/tests/protobuf_tests.cpp#L88:L104 which seems the way to generate protobuf message dynamically in the runtime rather than during the build.
>
> Benjamin Bannier wrote:
> I am not sure what I was referring to there, so I removed that line from the ticket. It seems just adding a rule to generate these output files and adding them to the test binary like we already do would do the job.
OK, I have posted a patch (https://reviews.apache.org/r/60109/) to generate `protobuf_tests.pb.*` during the build. I am not very familar with automake, so although the patch works, it may not be the right solution, please help review and let me know for any comments, thanks!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
On June 13, 2017, 4:41 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 4:41 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
> On June 12, 2017, 11:41 p.m., Zhitao Li wrote:
> > It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
> >
> > Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
>
> Qian Zhang wrote:
> Yes, the `.proto` change is the only real code changed, I have updated the description to mention that. And for the test, please refer to https://reviews.apache.org/r/59989/.
>
> Benjamin Bannier wrote:
> Could you have a look and try to find out how hard it would be to generate the `*.pb.*` files during the build instead of checking them in, https://issues.apache.org/jira/browse/MESOS-6115?
In MESOS-6115, I see you mentioned:
> We should try to remove them from the source tree; in fact protobuf_tests.cpp already documents ways this could be achieved.
I am not sure what you meant for `protobuf_tests.cpp already documents ways this could be achieved.`, did you mean the comments here: https://github.com/apache/mesos/blob/1.3.0/3rdparty/stout/tests/protobuf_tests.cpp#L88:L104 which seems the way to generate protobuf message dynamically in the runtime rather than during the build.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
On June 13, 2017, 4:41 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 4:41 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review177626
-----------------------------------------------------------
It seems that majority of this patch is generated code. Is the `.proto` change the only real code change? If so, should we write some c++ test code to use the map based fields and json parsing of map?
Also, explaining which files are generated in summary will help reviewers to know what changes we can safekly skip reading?
- Zhitao Li
On June 12, 2017, 2:06 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated June 12, 2017, 2:06 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new protobuf message `MapMessage` for protobuf tests.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
> 3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/#review186036
-----------------------------------------------------------
3rdparty/stout/tests/protobuf_tests.proto
Lines 102 (patched)
<https://reviews.apache.org/r/59988/#comment262411>
Please add tests for integral-typed keys.
- Chun-Hung Hsiao
On July 4, 2017, 2:44 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> -----------------------------------------------------------
>
> (Updated July 4, 2017, 2:44 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Zhitao Li.
>
>
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new protobuf message `MapMessage` for protobuf tests.
>
>
> Diffs
> -----
>
> 3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
>
>
> Diff: https://reviews.apache.org/r/59988/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
(Updated Dec. 19, 2017, 9:52 p.m.)
Review request for mesos, Anand Mazumdar and Zhitao Li.
Changes
-------
Addressed review comments.
Bugs: MESOS-7656
https://issues.apache.org/jira/browse/MESOS-7656
Repository: mesos
Description
-------
Added a new protobuf message `MapMessage` for protobuf tests.
Diffs (updated)
-----
3rdparty/stout/tests/protobuf_tests.proto cf8aadc7c003b3165421edb5ff2e71b6abf526f4
Diff: https://reviews.apache.org/r/59988/diff/5/
Changes: https://reviews.apache.org/r/59988/diff/4-5/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
(Updated July 4, 2017, 10:44 a.m.)
Review request for mesos, Anand Mazumdar and Zhitao Li.
Changes
-------
Rebased.
Bugs: MESOS-7656
https://issues.apache.org/jira/browse/MESOS-7656
Repository: mesos
Description
-------
Added a new protobuf message `MapMessage` for protobuf tests.
Diffs (updated)
-----
3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
Diff: https://reviews.apache.org/r/59988/diff/4/
Changes: https://reviews.apache.org/r/59988/diff/3-4/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
(Updated June 18, 2017, 8:42 p.m.)
Review request for mesos, Anand Mazumdar and Zhitao Li.
Bugs: MESOS-7656
https://issues.apache.org/jira/browse/MESOS-7656
Repository: mesos
Description
-------
Added a new protobuf message `MapMessage` for protobuf tests.
Diffs
-----
3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
Diff: https://reviews.apache.org/r/59988/diff/3/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
(Updated June 18, 2017, 8:40 p.m.)
Review request for mesos, Anand Mazumdar and Zhitao Li.
Changes
-------
Rebased
Repository: mesos
Description (updated)
-------
Added a new protobuf message `MapMessage` for protobuf tests.
Diffs (updated)
-----
3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
Diff: https://reviews.apache.org/r/59988/diff/3/
Changes: https://reviews.apache.org/r/59988/diff/2-3/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 59988: Added a new protobuf message `MapMessage`
for protobuf tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59988/
-----------------------------------------------------------
(Updated June 13, 2017, 4:41 p.m.)
Review request for mesos, Anand Mazumdar and Zhitao Li.
Changes
-------
Addressed Zhitao's comments.
Repository: mesos
Description (updated)
-------
The real code changes is adding `MapMessage` to protobuf_tests.proto,
for the other two files in this patch, they are automatically generated
by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
Diffs (updated)
-----
3rdparty/stout/tests/protobuf_tests.pb.h 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62
3rdparty/stout/tests/protobuf_tests.pb.cc ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6
3rdparty/stout/tests/protobuf_tests.proto d16726aa8060aea2b830040b20dbdd467c801483
Diff: https://reviews.apache.org/r/59988/diff/2/
Changes: https://reviews.apache.org/r/59988/diff/1-2/
Testing
-------
Thanks,
Qian Zhang