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