mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 48902: Move v1/master/allocator.proto to its own package.
Date Mon, 20 Jun 2016 16:45:41 GMT


> On June 20, 2016, 8:06 a.m., Alexander Rukletsov wrote:
> > To avoid confusion, we should actually change the namespace allocator code lives
is as well. I've once started that effort (https://reviews.apache.org/r/29930/) but decided
to discard because allocator is the master-only component.
> 
> Zhitao Li wrote:
>     Alexander, so what's your recommendation here? Given that there is only one public
visible message `InverseOfferStatus` in this protobuf at the moment, I'd like to have a quick
fix to make sure its namespace does not change after v1.0 and catch the release deadline.
>     
>     If "allocator is the master-only component" is considered a good reason to keep these
code in master namespace, then the question is how to handle maintenance which could have
circular dependency to master components: maybe we should just move this message to `mesos`
namespace as other high level messages like `Offer` or `TaskStatus`?
> 
> haosdent huang wrote:
>     >make sure its namespace does not change after v1.0 and catch the release deadline.
>     
>     +1 for this. I think all we don't want to break the V1 protobuf files after 1.0 release
>     
>     I am still curious why could not put allocator.proto and master.proto under the same
folder.
>     If it is forbidden and we want to keep allocator.proto under master namespace, I
think we could
>     move `message InverseOfferStatus` from allocator.proto to master.proto and then remove
allocator.proto.

@haosdent, the goal is here is to both make the protobuf structures consistent between packages,
and avoid possible circular dependency for various languages.

For golang, including a protobuf also generates an import to the package included protobuf
file is in. Therefore, `master` package imports `maintenance`, and `maintenance` imports `master`
(though this allocator.proto file) and causes a circular dependency.

After some thoughts, the only `InverseOfferStatus` message in allocator.proto definitely should
not belong to `master` package, because the latter is pretty much used soly for the new operator
HTTP API. Given that this message spans all "allocator", "Mesos master" and "maintenance"
logic concepts, it's common enough to be promoted to `include/mesos/v1/mesos.proto` and be
put next to `message InverseOffer`. And if we move this one, we can simply drop this allocator.proto
file because it'll be empty. (Alternatively, if we don't think it's common enough, moving
it to  `include/mesos/v1/maintenance/maintenance.proto` is acceptable to me).

Note that I'm only talking about the messages namespaced in v1. I agree we should make non
versioned messages consistent here, but that could be done in a separate patch.


- Zhitao


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


On June 18, 2016, 6:36 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48902/
> -----------------------------------------------------------
> 
> (Updated June 18, 2016, 6:36 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5642
>     https://issues.apache.org/jira/browse/MESOS-5642
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Move v1/master/allocator.proto to its own package.
> 
> 
> Diffs
> -----
> 
>   include/mesos/v1/maintenance/maintenance.proto 053d72d7d9fa9ec8f572136020546fa4f955ab09

>   include/mesos/v1/master/allocator.proto cf416d173bc487aecbbec75c9ee391a54bf5327b 
>   src/CMakeLists.txt d66186217c1319d4497640614ed4beee28602c38 
>   src/Makefile.am a4931560f1a5b3fbe41ea181477341d3ac459b58 
> 
> Diff: https://reviews.apache.org/r/48902/diff/
> 
> 
> Testing
> -------
> 
> run `make` on Mac.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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