> On Feb. 18, 2015, 9:28 p.m., Jie Yu wrote: > > include/mesos/module/module.hpp, lines 29-40 > > <https://reviews.apache.org/r/31162/diff/1/?file=868266#file868266line29> > > > > Please move this to common/type_utils.hpp
`common/type_utils.hpp` has been renamed to `include/mesos/type_utils.hpp`. The reason for moving this code is to make type_utils.hpp independent of module.hpp. I imagine type_utils.hpp being closely tied up with mesos.proto and so I think we should keep it as it. The other piece that I didn't like about the existing code was the introduction of "namespace internal" in type_utils.hpp. With this change, the "internal" namespace is localized to modules.proto and modules.hpp. However, if there is a strong preference, I can undo to code move. > On Feb. 18, 2015, 9:28 p.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? > > Benjamin Hindman wrote: > +1, why the code movement of this? See the comment above. > On Feb. 18, 2015, 9:28 p.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)? > > Benjamin Hindman wrote: > Regardless, we shouldn't have the 'using' in the headers, so we should be > keeping this here IIUC. Dropped, coz the new patch doesn't have `using` in messages.hpp. > On Feb. 18, 2015, 9:28 p.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. > > Benjamin Hindman wrote: > +1 The string "package.messagename" is generated by protobuf compiler and we get it when we do "message->name". So we have to stick with this one. > On Feb. 18, 2015, 9:28 p.m., Jie Yu wrote: > > src/common/parse.hpp, line 61 > > <https://reviews.apache.org/r/31162/diff/1/?file=868269#file868269line61> > > > > This is a little unfortunate, could you please add a TODO here and link > > the ticket? I am not sure if I understand the comment. Also, now that we are adding "internal::" all over the place, I guess this issue becomes void. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31162/#review73066 ----------------------------------------------------------- On Feb. 19, 2015, 5:59 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31162/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2015, 5:59 p.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/authenticatee.hpp > 17fb7aa0f4d8a43f9cee0aab305af05f4fa7888f > include/mesos/authentication/authentication.proto > 38a6f781d6a0b4618a14e1681420564d78b840a8 > include/mesos/authentication/authenticator.hpp > f66217a2ee16c76a0b71e0b3057142ba3f9fb6e4 > include/mesos/module/module.hpp e83be2822b7c0e7935ab1c8af36e5cb9b5180f20 > include/mesos/module/module.proto 821fc0e72ece7c497595859fc5efc1c64ea49b9b > include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 > src/authentication/cram_md5/authenticatee.hpp > 33119160cea0bdf41a820c61c6fcbef7766d7ba3 > src/authentication/cram_md5/authenticator.hpp > 7578ea160903206628672f5ed84b6032c9a38c0a > src/cli/resolve.cpp 77bd4439f696bf16407a84243980834080af8f88 > src/common/http.hpp bdb5948a850ac9dda066dcb895c092d2108f62a5 > src/common/http.cpp 4b6040837a1532b3264a1a3a38b618841cc19961 > src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc > src/common/protobuf_utils.hpp 82c6e4f693fe3e02d76d2865702d79f28c3e19b5 > src/common/protobuf_utils.cpp c398c58511d74c33f24002f79c01fcd45842df36 > src/common/type_utils.cpp 12a36bbd7d7773b25dedf2d0d951c79e0b5141d6 > src/exec/exec.cpp 8250bd4ec9028fb4667b9c703a2bb780fbf2171e > src/health-check/main.cpp 260b872272955f7ee20c0cc1d53d578d7ad2b731 > src/launcher/executor.cpp f5db28cc155691f50cc00484db48a04502face8c > src/log/catchup.cpp c33ecd85f6d0d78e7d96e4c75ac48c9e6bd60827 > src/log/consensus.hpp ee9e9081ffe2a5f18433d9eff1a2e2cf17c81863 > src/log/consensus.cpp 7e33c693259cdb9dc359a7d25219b2047df390d9 > src/log/coordinator.cpp b97e133a7458b57dd94b99af385585cec5b2de8d > src/log/leveldb.hpp 8f5df5bdfb08de02e4129494fba188f97c3b8010 > src/log/leveldb.cpp 1d679425c6df3498be67d048bc05d02ffe03a636 > src/log/log.cpp cd7b6a8b1f1471b8f0ab198de646fa7be18c4af5 > src/log/recover.cpp 3e175f3bbd598230c54f3d1a5cda2ca6e137c6a8 > src/log/replica.hpp b2602083564447cf75827257a2548c1b5aeb49e2 > src/log/replica.cpp 83cb3e7d9271ab4e4c9e0df98824aa933190a4d9 > src/log/storage.hpp 5e81f4e9774ad239145019320cf991fcf51a9da5 > src/log/tool/initialize.cpp f3d6337f6f7817ec6e1bc8b3a2481c1f6a1a8250 > src/log/tool/read.cpp 4c6c30db5dbd5bde2cf3b452b07a6b4011d39b99 > src/master/allocator/allocator.hpp c2461a32e18dbafe637f37180f86bfa2e1d78735 > src/master/allocator/mesos/allocator.hpp > 99b40a01749b77fc7c1fa025a7ac81d5cca6272f > src/master/allocator/mesos/hierarchical.hpp > 2680d6231927867d5a8d75cbc42b81d6c75fc7f2 > src/master/detector.cpp 053b0a7e1b62bfe11e263cff43f5d155580d8696 > src/master/flags.hpp 51a6059b05be25cc34f6a618da27dbca59144cfc > src/master/http.cpp 5e3afdf8f32a7bf146eb39e53d1543c92e6180ba > src/master/master.hpp 6a39df04514c756415354fae66c5835ada191c52 > src/master/master.cpp f10a3cffdb0b9831b2b8af0dfd30f73d1d8ed937 > src/master/registrar.hpp 701b1071bbad35872bbf924491d658899396c810 > src/master/registrar.cpp 2bfc053595cf93890d19ca4ccd9a4a1ee44edc26 > 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/module/manager.hpp f071678aca0729c14147b16f00a6f1e0daf9094e > src/module/manager.cpp 44a3e6587d6dbf8f75f8e3f41896f68a77362619 > src/sched/flags.hpp bfe01a32db2cdd9783902a418c16ff5e88cadc1b > src/sched/sched.cpp ea7e447e522f8fa335ee5dbdc6d65d4018042905 > src/scheduler/scheduler.cpp 5816569dc9cfeb86fe59b8fbea1518825b403182 > src/slave/flags.hpp ddb32593566b71bcf0b66adeeb889e43ef911084 > src/slave/http.cpp ef6e64882282a9d7be252ccdc545ab96d6323c3b > src/slave/main.cpp 173a1a3117901d61027fe27d5f411312ed9d9777 > src/slave/paths.cpp cf08c445b772cc1a98e8abdcb3db7be098035633 > src/slave/slave.hpp 91dae1074607993f85628d3ba3cde485097bd873 > src/slave/slave.cpp aec9525d7af1cc7383f1babeb7867cd8a63d1a4c > src/slave/state.hpp e7276a503d54075f3eddd2e890e0aa4f3721ea00 > src/slave/state.cpp 24ab12c7f06000c3b04e6a826b5b891e2a9c2dfe > src/slave/status_update_manager.hpp > 44b60b508c2556537e3fc9358257b2a3d10cd4c2 > src/slave/status_update_manager.cpp > 5b0192b688bdeb8cf3411da7aedbee72caf50a7d > src/state/in_memory.hpp fea2e107dd6521fee3e45fbc69480f03513aff32 > src/state/in_memory.cpp 6f336161a2269866ee18ef0241cb8a121c2b93cf > src/state/leveldb.hpp 379316ce150ef98248e7384bc52a9f2924a0dc7f > src/state/leveldb.cpp 263a6b99bb405ea52f3abc608cb5180404854c55 > src/state/log.hpp 1558dad180015bbf1867d4d443fa1f2759fc9d37 > src/state/log.cpp 6069d129886095f0a08d3c15ca13703a70a97dc7 > src/state/state.hpp 878265da5556b0b43a519ba21de9f522eb71b88c > src/state/storage.hpp f5cd607b47d44f72d72cbd0c5adb7368410b83d8 > src/state/zookeeper.hpp 9352a2182d21f04ea66a0a2a858e3a940754109a > src/state/zookeeper.cpp e570a9b82de25025ee06350b7c54e86a27392c4b > src/tests/authentication_tests.cpp 59d29549b2831a0a50478456d16fdc41022a221e > src/tests/cgroups_tests.cpp 9d50a47c939f5f136c85fdcc555459512c6f2259 > src/tests/common/http_tests.cpp b030a783c3932912dd431ad1fe24f488aa5d9460 > src/tests/composing_containerizer_tests.cpp > 6bb49ea780b0fe79974e57b35f886ee82a1e3f1c > src/tests/cram_md5_authentication_tests.cpp > dd102dca6f37cc4a7d8c8a67bfb05bbe0e7da3b5 > src/tests/credentials_tests.cpp 4210d315bae423cf27d181a633090c99d2f5bc62 > src/tests/docker_containerizer_tests.cpp > 8b212d4e6ed5a179ebadce1bdbbf3edde87d706c > src/tests/exception_tests.cpp 7a41770c26519541fed10672e13ee833a2d6dd5e > src/tests/fault_tolerance_tests.cpp > efa5c57d98308e9d54723d361533b39b99551261 > src/tests/flags.hpp 3155d89b9c42a0e3b650703d7a0c553c1b00e010 > src/tests/gc_tests.cpp c74287a29eaa8478c301ef3bcfd9af1e27a67b7e > src/tests/hierarchical_allocator_tests.cpp > eeecfb64540b16666915074aaffaa5d506b203bc > src/tests/log_tests.cpp 8cbc4df5dac49eef4ab618f47862812266ed513d > src/tests/main.cpp 0ab6be502e3c2c23b728c27dd5cbdd014da08cd5 > src/tests/master_allocator_tests.cpp > df9ac24280514932dc118408b11adc806f548f5e > src/tests/master_authorization_tests.cpp > fadbd38f50c9587a9b1755d96a96e10c540264c5 > src/tests/master_contender_detector_tests.cpp > 78b1a6a393a0c69ade12d7f74ee931254b9975db > src/tests/master_slave_reconciliation_tests.cpp > 5156898a4ec02a2ad0080ab768e360077f0ba0f1 > src/tests/master_tests.cpp 3a9da68364fda25ac25565a1d2799f2dfaf736da > src/tests/master_validation_tests.cpp > e2b64267cb0df5c9944f27d01af90ae15a1f2432 > src/tests/mesos.hpp 60c70043c8266a422ffa03ab5a949da0bc822124 > src/tests/module.hpp 5748add67ef7d194c7ca8521f505079b617ac139 > src/tests/module.cpp 3c9bab7a192a63adfa3cfc8fd1e29dada00f8095 > src/tests/module_tests.cpp a16ba994951ef581d703e96b14f4d95f483cb2cf > src/tests/partition_tests.cpp eb16a58c23e0c152212140841ed09cd7f6f139d6 > src/tests/paths_tests.cpp 07e8af56ea7e186b7daff8c7695e997edf325ad7 > src/tests/persistent_volume_tests.cpp > b4f2298069233b27f6b4b8d668753810f43bb58d > src/tests/protobuf_io_tests.cpp 92a090a221bcbdc7adc4ca3c717302c48c57902d > src/tests/rate_limiting_tests.cpp 784ac76ee81918466de44196f5adc2b88adfee96 > src/tests/reconciliation_tests.cpp 4c97341a1e84b1504090e8b3fdd8b309abf65877 > src/tests/registrar_tests.cpp fd312930e33e332c3d5b6291c191050378694d9e > src/tests/repair_tests.cpp 1aad5c15a1c1111c481d5d97f6f65b6ba1f3706c > src/tests/resource_offers_tests.cpp > 1ba73ba7f9c2cd042119a2bd47c16f152ebc65fa > src/tests/resources_tests.cpp 3f98782fd437dba808d720bf8e9b94b8fa7e0feb > src/tests/scheduler_tests.cpp 9071a852bc0255a9400f88834d0ad3b3a7463c3a > src/tests/shutdown_tests.cpp 9017517bf7a5a79c5d4ca63fc1b23b2c5403b538 > src/tests/slave_recovery_tests.cpp 8210c52afd03e2a722657c45ef8ed2dae7988022 > src/tests/slave_tests.cpp 153d9d625a4a843d21e4cd6ea3b4fa06c4c440f2 > src/tests/state_tests.cpp b30812322dd8ae507e1d45912ab559940ac5ee73 > src/tests/status_update_manager_tests.cpp > 2158cfea98d238d462abeebd3d5fe2dec4f96470 > > 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 > >
