> On Nov. 4, 2016, 12:30 p.m., Neil Conway wrote: > > src/master/validation.cpp, line 1532 > > <https://reviews.apache.org/r/52587/diff/3/?file=1550774#file1550774line1532> > > > > Can we use consistent tense here? The other volume error messages say > > "has been attempted" rather than "being attempted". > > > > Also, the other error messages don't include the volume that failed > > validation, so we should probably be consistent. > > Jiang Yan Xu wrote: > +1 on the tense. > > but re: "the other error messages don't include the volume that failed > validation" they don't include the volume but they include other things this > error message doesn't include. Whether the additional information is useful > depends on the failure IMO. Nevertheless I guess it's true we don't have to > say it's a shared volume separately because `stringify(volume)` reveals it. > > > So overall the following look good? > > ``` > ... > > "Create volume operation for '" + stringify(volume) + > "' has been attempted by framework '" + > > ... > ``` > > Neil Conway wrote: > Why do you think this particular error message warrants including > `stringify(volume)` any more/less than the other places where a volume fails > validation? > > I'm not opposed to including more context in validation failure messages, > but I think we should also be consistent.
My critiera for adding `stringify(volume)` is that it will print the sharedness of the volume so it's obvious in the log w.r.t the operation: which volume is problematic, what makes it problematic, why it is problematic. However examining the two other error messages closely, I think they should be given the same treatment. I'll send a RR. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52587/#review154975 ----------------------------------------------------------- On Nov. 1, 2016, 9:54 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52587/ > ----------------------------------------------------------- > > (Updated Nov. 1, 2016, 9:54 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6316 > https://issues.apache.org/jira/browse/MESOS-6316 > > > Repository: mesos > > > Description > ------- > > When a framework issues a CREATE for a shared volume, we allow that > operation only if the framework has opted in for the capability of > SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we > do not check as the operator API is not related to a specific > framework. > > > Diffs > ----- > > src/master/master.cpp 013bb592ba47b785c552e199633e4784e8aa71b1 > src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 > src/master/validation.cpp f690a9eacd278b51a52f5588dbeea377df074435 > src/tests/master_validation_tests.cpp > a5d8610bd61822cdf55cbc8d7056e5cf8fecfa54 > src/tests/persistent_volume_tests.cpp > 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 > > Diff: https://reviews.apache.org/r/52587/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
