mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 59988: Added a new protobuf message `MapMessage` for protobuf tests.
Date Thu, 15 Jun 2017 07:10:55 GMT


> 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
> 
>


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