mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vijay Srinivasaraghavan <vijikar...@yahoo.com>
Subject Re: Review Request 53825: MESOS-6597 Enabled java protos generation for all V1 proto files.
Date Wed, 23 Nov 2016 04:44:05 GMT


> On Nov. 20, 2016, 9:04 p.m., Anand Mazumdar wrote:
> > Nice first patch and welcome to the community!
> > 
> > - Can you update the Testing Done section with details on testing?
> > - We also support Python bindings. Do you mind adding these protos to our python
build too in a follow up review? (See my comments later on how you can use review dependencies
if you are new to ReviewBoard)
> 
> Vijay Srinivasaraghavan wrote:
>     Thanks Anand. I have made the python binding changes locally on my machine. Do you
want me to create a new JIRA bug for this issue and create a new review or add the patch to
this JIRA/review itself? I am little confused as to how the dependencies are handled with
RB.
> 
> Anand Mazumdar wrote:
>     No need to create a separate issue. It would be fine to create a new review with
it only containing the python related changes. This would be another commit based on top of
this one. You can then use `post-reviews.py` and it should handle setting the dependencies
for you. Feel free to ping me on IRC/Slack if you run into any issues (anandm)

Thanks for the clarification. I have attached a new patch with "53825" as dependendency


> On Nov. 20, 2016, 9:04 p.m., Anand Mazumdar wrote:
> > include/mesos/v1/allocator/allocator.proto, lines 21-22
> > <https://reviews.apache.org/r/53825/diff/2/?file=1566692#file1566692line21>
> >
> >     These changes seem unrelated to this change i.e., java protos generation. We
prefer single logical atomic commits in Mesos. For more info see: http://mesos.apache.org/documentation/latest/submitting-a-patch/
> >     
> >     Can you create a separate patch for this and make this review dependent on it?
The `post-reviews.py` script would do it automatically for you.
> 
> Vijay Srinivasaraghavan wrote:
>     Actually, this fix is also part of the same issue. The proto file was missing specific
java package definition and hence the java file gets generated in a package location different
from the other java files.
> 
> Anand Mazumdar wrote:
>     hmm, are they the same? They are two separate issues:
>     
>     1. The proto file was missing specific java package definition. This would impact
any user trying to use the proto file and is not related to including the generated files
in the Mesos JAR.
>     2. Include the generated files in the Mesos JAR.
>     
>     This review (from its summary/description) caters to _only_ build changes in the
MakeFile itself and not protobuf related changes.

I see your point. I have attached new patch/review marking "53825" as dependent.


- Vijay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53825/#review156423
-----------------------------------------------------------


On Nov. 23, 2016, 4:31 a.m., Vijay Srinivasaraghavan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53825/
> -----------------------------------------------------------
> 
> (Updated Nov. 23, 2016, 4:31 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zameer Manji.
> 
> 
> Bugs: MESOS-6597
>     https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> MESOS-6597 Enabled java protos generation for all V1 proto files.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/53825/diff/
> 
> 
> Testing
> -------
> 
> Built mesos and confirmed jar file containing new java PB wrappers for V1 API. Ran standard
unit test (make tests)
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>


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