> On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/tests/state_tests.cpp, lines 86-87 > > <https://reviews.apache.org/r/31162/diff/1/?file=868284#file868284line86> > > > > typedef Registry::Slaves Slaves; > > typedef Registry::Slave Slave;
This reverts the code to it's original state which is most explicit, let's leave it as it is. > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/tests/rate_limiting_tests.cpp, line 1058 > > <https://reviews.apache.org/r/31162/diff/1/?file=868283#file868283line1058> > > > > 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. +1 > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/messages/messages.hpp, lines 73-107 > > <https://reviews.apache.org/r/31162/diff/1/?file=868279#file868279line73> > > > > Curious why these are not in type_utils? I'm assuming this was not part of Kapil's change, so let's move them later. Dropping this issue for now. > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/log/leveldb.cpp, lines 42-43 > > <https://reviews.apache.org/r/31162/diff/1/?file=868273#file868273line42> > > > > Ditto above. This one is in a .cpp file, dropping. > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/log/consensus.hpp, lines 44-47 > > <https://reviews.apache.org/r/31162/diff/1/?file=868271#file868271line44> > > > > 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. Do we need any "hacks" here? If so, we need to CLEARLY call them out with comments. Ideally we don't need any hacks, please point me to any hacks. > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > src/common/type_utils.cpp, lines 160-161 > > <https://reviews.apache.org/r/31162/diff/1/?file=868270#file868270line160> > > > > I am wondering why this is necessary if you already included > > messages/messages.hpp (which has using namespace mesos::internal)? Regardless, we shouldn't have the 'using' in the headers, so we should be keeping this here IIUC. > On Feb. 19, 2015, 2:28 a.m., Jie Yu wrote: > > include/mesos/type_utils.hpp, lines 377-384 > > <https://reviews.apache.org/r/31162/diff/1/?file=868268#file868268line377> > > > > 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? +1, why the code movement of this? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31162/#review73066 ----------------------------------------------------------- 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 > >
