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