mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 31162: Backout change 30300 (Move internal protos from mesos::internal to mesos namespace)
Date Thu, 19 Feb 2015 02:28:07 GMT

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


The high level comment is: you should only put the namespace hacks in protobuf wrapper headers
so that once we move forward, we can just go to all the protobuf header wrappers and remove
those hacks. Rest the code base should not have any "internal::" namespace (maybe one exception
which is unfortunate).


include/mesos/authentication/authentication.hpp
<https://reviews.apache.org/r/31162/#comment119249>

    Could you expand the comments here explaining why we need to do this include? Maybe link
the ticket as well. Here and everywhere else.



include/mesos/module/module.hpp
<https://reviews.apache.org/r/31162/#comment119250>

    Please move this to common/type_utils.hpp



include/mesos/type_utils.hpp
<https://reviews.apache.org/r/31162/#comment119248>

    Instead of pulling it into module.hpp, can you keep it here? Also, if you include module/module.hpp,
do you still need to change this code?



src/common/parse.hpp
<https://reviews.apache.org/r/31162/#comment119263>

    This is a little unfortunate, could you please add a TODO here and link the ticket?



src/common/type_utils.cpp
<https://reviews.apache.org/r/31162/#comment119251>

    I am wondering why this is necessary if you already included messages/messages.hpp (which
has using namespace mesos::internal)?



src/log/consensus.hpp
<https://reviews.apache.org/r/31162/#comment119253>

    Let's keep all the namespace hacks in protobuf wrapper headers so that it's easy for us
to find them later. In this case, you can put `using namespace mesos::internal::log` in messages/log.hpp.



src/log/leveldb.hpp
<https://reviews.apache.org/r/31162/#comment119260>

    Ditto above.



src/log/leveldb.cpp
<https://reviews.apache.org/r/31162/#comment119259>

    Ditto above.



src/log/replica.hpp
<https://reviews.apache.org/r/31162/#comment119254>

    Ditto above.



src/messages/messages.hpp
<https://reviews.apache.org/r/31162/#comment119256>

    Curious why these are not in type_utils?



src/state/storage.hpp
<https://reviews.apache.org/r/31162/#comment119258>

    Let's do the similiar thing here (like the log above). Kill this and put that into messages/state.hpp.



src/tests/rate_limiting_tests.cpp
<https://reviews.apache.org/r/31162/#comment119261>

    I would suggest just change this comment to not include the namespace so that we don't
forget to change this once we move forward.
    ```
    Message RegisterFrameworkMessage ...
    ```
    
    Please do a sweep to find all such cases and fix them.



src/tests/state_tests.cpp
<https://reviews.apache.org/r/31162/#comment119262>

    typedef Registry::Slaves Slaves;
    typedef Registry::Slave Slave;


- Jie Yu


On Feb. 19, 2015, 12:59 a.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31162/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2015, 12:59 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Till Toenshoff, and Vinod
Kone.
> 
> 
> Bugs: MESOS-2371
>     https://issues.apache.org/jira/browse/MESOS-2371
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This changeset puts the internal protos back into mesos::internal namespace.
> This is required for upgrade compatibility.  Without it, a newer Master would
> reject all messages from an older Slave and vice-versa.
> 
> https://reviews.apache.org/r/30300/
> 
> 
> Diffs
> -----
> 
>   include/mesos/authentication/authentication.hpp 699aa886286bc7d9c05592e71232ab1c1084871f

>   include/mesos/authentication/authentication.proto 38a6f781d6a0b4618a14e1681420564d78b840a8

>   include/mesos/module/module.hpp e83be2822b7c0e7935ab1c8af36e5cb9b5180f20 
>   include/mesos/module/module.proto 821fc0e72ece7c497595859fc5efc1c64ea49b9b 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
>   src/common/type_utils.cpp 12a36bbd7d7773b25dedf2d0d951c79e0b5141d6 
>   src/log/consensus.hpp ee9e9081ffe2a5f18433d9eff1a2e2cf17c81863 
>   src/log/leveldb.hpp 8f5df5bdfb08de02e4129494fba188f97c3b8010 
>   src/log/leveldb.cpp 1d679425c6df3498be67d048bc05d02ffe03a636 
>   src/log/replica.hpp b2602083564447cf75827257a2548c1b5aeb49e2 
>   src/log/storage.hpp 5e81f4e9774ad239145019320cf991fcf51a9da5 
>   src/master/registry.hpp d3cbe564dc8e14f3dd301e5bc06ed60d82856eee 
>   src/master/registry.proto 29a309763bca9db76c443d7c039ca152a2afbc5b 
>   src/messages/log.proto 12b3572e0fe0e30c5eff20c7af27eb3d7cfe8f4b 
>   src/messages/messages.hpp 25769b797ec9bd1a1c348c467616aef8407c45d6 
>   src/messages/messages.proto 58484ae45071a80afd2b11803dd66a88f88ad9ed 
>   src/messages/state.proto 7fc4883b9fe590afbbb0261988c929c3b1f8f676 
>   src/state/storage.hpp f5cd607b47d44f72d72cbd0c5adb7368410b83d8 
>   src/tests/rate_limiting_tests.cpp 784ac76ee81918466de44196f5adc2b88adfee96 
>   src/tests/state_tests.cpp b30812322dd8ae507e1d45912ab559940ac5ee73 
> 
> Diff: https://reviews.apache.org/r/31162/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> Also tested with master running HEAD with this patch, and slave running 0.21.0 with src/test-framework.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>


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