----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102110 -----------------------------------------------------------
Looking good, but I haven't made it to the tests yet (coming soon), and I assume the v1 resources are identical? Can you elaborate on the "Testing Done"? Call out any new/modified unit tests, or any existing tests that verify your changes did not affect functionality? include/mesos/resources.hpp (line 80) <https://reviews.apache.org/r/39018/#comment159629> "a Resources object containing a single resource"? Looks like it's actually "a single Resource object". Maybe just s/Resources/Resource/? include/mesos/resources.hpp (lines 83 - 98) <https://reviews.apache.org/r/39018/#comment159678> Do these all need to be public? include/mesos/resources.hpp (line 98) <https://reviews.apache.org/r/39018/#comment159639> s/checkParsedResource/validateParsedResource/? src/common/resources.cpp (line 362) <https://reviews.apache.org/r/39018/#comment159681> You call `resourcesTry.get()` 4 times in this block. Maybe worth a `JSON::Value resourcesValue = resourcesTry.get();` above? src/common/resources.cpp (lines 368 - 384) <https://reviews.apache.org/r/39018/#comment159684> This smells like a hack to workaround a bug in picojson. Can you link to something that indicates this as best practice or a recommended workaround? Is there a picojson issue we can track to know if this ever gets fixed? src/common/resources.cpp (line 482) <https://reviews.apache.org/r/39018/#comment159696> Only needed just before the `foreach`. Might be unnecessary if protobuf::parse errors, so let's move it down to where it's used. src/common/resources.cpp (lines 501 - 508) <https://reviews.apache.org/r/39018/#comment159692> Duplicated code. Can you factor this out? src/common/resources.cpp (line 522) <https://reviews.apache.org/r/39018/#comment159697> s/validation/validationError/ (or just `error`) because "validation.isSome()" sounds like a good thing. src/common/resources.cpp (line 535) <https://reviews.apache.org/r/39018/#comment159698> Disk resources are fine. Just not a disk with a 'persistence' field. src/common/resources.cpp (line 632) <https://reviews.apache.org/r/39018/#comment159694> No longer needs explicit namespace, since we're `using RepeatedPtrField`. Ditto for the 2 other uses in this file. - Adam B On Oct. 7, 2015, 8:24 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39018/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2015, 8:24 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael > Park. > > > Bugs: MESOS-2467 > https://issues.apache.org/jira/browse/MESOS-2467 > > > Repository: mesos > > > Description > ------- > > This includes code changes necessary for JSON parsing of Resources. > Documentation changes will be posted soon in another review. > > > Diffs > ----- > > include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 > include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 > src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 > src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 > src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 > > Diff: https://reviews.apache.org/r/39018/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Greg Mann > >
