----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36718/#review92785 -----------------------------------------------------------
Overall looks good to me! I wish we could merge all the protobuf related rules in src/Makefile.am as we are going to introduce more and more protobuf definitions. include/mesos/slave/isolator.proto (line 30) <https://reviews.apache.org/r/36718/#comment147037> `slave::Limitation` sounds a little too broad. How about calling it `slave::ExecutorLimitation`? include/mesos/slave/isolator.proto (line 38) <https://reviews.apache.org/r/36718/#comment147027> Why a default here? It's confusing between not setting the message and a default empty string. I would suggest you remove the default here. include/mesos/slave/isolator.proto (line 51) <https://reviews.apache.org/r/36718/#comment147025> s/id/container_id/ src/Makefile.am (lines 157 - 165) <https://reviews.apache.org/r/36718/#comment147031> Not your fault, but please sort those PROTO definitionss alphebetically. src/Makefile.am (lines 183 - 184) <https://reviews.apache.org/r/36718/#comment147032> Ditto here. src/Makefile.am (line 278) <https://reviews.apache.org/r/36718/#comment147036> As we keep adding more protobuf files, this will soon become unmanagable. I am wondering if we can combine these rules. I think we can have a single variable PROTOS which keeps all .proto files. I think we should be able to write a rule that can combline all the rules here. src/common/protobuf_utils.cpp (lines 209 - 238) <https://reviews.apache.org/r/36718/#comment147038> Hum, these are slave specific. How about putting them in protobuf::slave namespace? src/slave/containerizer/isolators/posix/disk.cpp (line 62) <https://reviews.apache.org/r/36718/#comment147039> No need to do this. ``` protobuf::createExecutorLimitation ``` reads better. src/slave/containerizer/mesos/containerizer.cpp (line 85) <https://reviews.apache.org/r/36718/#comment147040> Ditto. - Jie Yu On July 23, 2015, 3:37 a.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36718/ > ----------------------------------------------------------- > > (Updated July 23, 2015, 3:37 a.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Bugs: MESOS-3115 > https://issues.apache.org/jira/browse/MESOS-3115 > > > Repository: mesos > > > Description > ------- > > Protobufs are preferred over C structs for public API. > > > Diffs > ----- > > include/mesos/slave/isolator.hpp 85e38f5e4aa66527f1756fa259b93389f45028b3 > include/mesos/slave/isolator.proto PRE-CREATION > src/Makefile.am 489ddb424b342635c3dbc4d14ff5d69ce76a237b > src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 > src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 > src/exec/exec.cpp a1ae074b962d8e93ab7776bd624389857da486f3 > src/slave/containerizer/isolators/cgroups/cpushare.cpp > 750bef947c11eb55236ac46109b9dd97e62b453d > src/slave/containerizer/isolators/cgroups/mem.cpp > b0e343fdc7088b2895d5dc8bb416dbcbf241cae5 > src/slave/containerizer/isolators/cgroups/perf_event.cpp > 512df3be7fdf6bac22ad4122f54a21d9986a1a6a > src/slave/containerizer/isolators/filesystem/posix.cpp > 1904279c92ef00ef931c909b4bb15bef89a4fc59 > src/slave/containerizer/isolators/namespaces/pid.cpp > 5de0791a835d725b7c7aae1ba585a94cff9372f1 > src/slave/containerizer/isolators/network/port_mapping.cpp > a7757f2a51f04da27645074f048722c22a2be752 > src/slave/containerizer/isolators/posix.hpp > 271061ef97aea96bb816982e530c84554d4b08d8 > src/slave/containerizer/isolators/posix/disk.cpp > b2f995cba36b1399db48af1de49d76c607f80abd > src/slave/containerizer/launcher.cpp > 24df1ca5f7407062f9e7b4bfa18f2cae5c72e140 > src/slave/containerizer/linux_launcher.cpp > 790e392645dd62e74b03ff0771f6bf0e9efeb622 > src/slave/containerizer/mesos/containerizer.cpp > 609620c4322e41562597ee682b311cd320bca6d2 > > Diff: https://reviews.apache.org/r/36718/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Kapil Arya > >
