mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 48902: Move v1/master/allocator.proto to its own package.
Date Mon, 20 Jun 2016 20:46:04 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.
> 
> Zhitao Li wrote:
>     @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.
> 
> haosdent huang wrote:
>     Got it, +1 for move `InverseOfferStatus` to `package mesos` which follow `InverseOffer`.
>     Hi, @alexr Do you think if this is OK?

It is fine to do the v1 change. I would like to avoid inconsistency, when some allocator-related
types live in "master" namespace or package, and some in "allocator". Before you move, could
you ask Joris or Joseph why they had put `InverseOfferStatus` into a separate proto in the
first place?


- Alexander


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