> On Aug. 4, 2015, 9:16 p.m., Artem Harutyunyan wrote: > > src/tests/containerizer/port_mapping_tests.cpp, line 986 > > <https://reviews.apache.org/r/36979/diff/1/?file=1026038#file1026038line986> > > > > ditto. > > + extra newline. > > Marco Massenzio wrote: > Having looked at both tests, I was being unnecessarily pedantic IMO: > checking for the error code (256) to be present in the error string seems to > me to be more than sufficient (and self-explanatory too - but added a comment > all the same). > > What thinks you?
After think a bit more about it, I find it a bit unusual that the user has to perform a string search in order to get out the integer error code. In cases when you expect a certain kind of failure from a certain command it's easy (like in your test case), but what if the cause of failure is unknown, or if there are several possible error codes expected. It looks to me that one will need to involve a regex parser to be able to reliably(?) get the signal and the error code out. This might drive delopers away from this function, and cause proliferation of similar code in the codebase (something that this was meant to facilitate avoiding). Returning a primtive struct(or a union) with a couple of fields could easily help to avoid that. - Artem ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94177 ----------------------------------------------------------- On Aug. 5, 2015, 10:10 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36979/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 10:10 a.m.) > > > Review request for mesos, Benjamin Hindman and Artem Harutyunyan. > > > Bugs: MESOS-3142 > https://issues.apache.org/jira/browse/MESOS-3142 > > > Repository: mesos > > > Description > ------- > > Updating all references to os::shell > For more details see MESOS-3142. > > > Diffs > ----- > > src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 > src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce > src/slave/containerizer/isolators/network/port_mapping.cpp > 8244c345b84108af7fa18d20e71401d6e1a0aeb0 > src/tests/containerizer/isolator_tests.cpp > ff6e2b7e190a58a4809d6e71addb15dabe418e17 > src/tests/containerizer/port_mapping_tests.cpp > 4bee74acba2b1472c80cabbc9d0384bd04c543aa > > Diff: https://reviews.apache.org/r/36979/diff/ > > > Testing > ------- > > make check > *Note*: this patch fixes breakages introduce by the refactoring in: > https://reviews.apache.org/r/36978 > > > Thanks, > > Marco Massenzio > >
