> On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.hpp, line 753 > > <https://reviews.apache.org/r/24343/diff/1/?file=653285#file653285line753> > > > > is there a reason this is a nested class in Master?
It only intended to be used in Master but I guess it doesn't need to be defined in Master. Existing "internal" classes and structs defined in master.hpp are defined both inside and outside but the ones outside are accessed from http.cpp as well so I figured that's the reason. In this case I just thought it logically belongs "inside". Any reason not to be put inside as nested? > On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.hpp, line 766 > > <https://reviews.apache.org/r/24343/diff/1/?file=653285#file653285line766> > > > > might be worth adding the capacity to the error message to make it > > easier to debug issues. Done. > On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 939 > > <https://reviews.apache.org/r/24343/diff/1/?file=653286#file653286line939> > > > > maybe a const Option<string>& here? Thanks! > On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 377 > > <https://reviews.apache.org/r/24343/diff/1/?file=653286#file653286line377> > > > > please pull these out into more traditional if statements above the > > 'put' call. this is completely unreadable. Done. > On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 884 > > <https://reviews.apache.org/r/24343/diff/1/?file=653286#file653286line884> > > > > assuming there might be more errors in the future, this comment might > > be misleading. Removed the comment and now pass the returned error to __visit() for better logging and error message sent to framework. > On Aug. 5, 2014, 4:50 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 997 > > <https://reviews.apache.org/r/24343/diff/1/?file=653286#file653286line997> > > > > consider including the number of messages and principal. Included the "number of messages", i.e., the capacity. Did not include the principal as I think it should be very clear to the framework what its own principal is (if it sets it in FrameworkInfo) and it could be empty (if it doesn't set it). "capacity" on the other hand is configured on the master so I think it's useful information. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24343/#review49664 ----------------------------------------------------------- On Aug. 5, 2014, 10:53 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24343/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2014, 10:53 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-1578 > https://issues.apache.org/jira/browse/MESOS-1578 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > include/mesos/mesos.proto 6d4fd145004a14514e27ba65b2eae19b945aeb83 > src/examples/load_generator_framework.cpp > 7d94c49cf91bf327ac80f04d9f1a7370996b6ba4 > src/master/master.hpp d8a4d9e04ecff60020b99ea6447055787d187797 > src/master/master.cpp 9bede96083f56768a5d42baf051af97de4c48d48 > src/tests/rate_limiting_tests.cpp fc23a1946ad1a78e699552440df2193ea10dc472 > > Diff: https://reviews.apache.org/r/24343/diff/ > > > Testing > ------- > > make check > > ./bin/mesos-tests.sh --verbose --gtest_filter=*RateLimit* --gtest_repeat=1000 > > > Thanks, > > Jiang Yan Xu > >
