Re: Review Request 51999: Refactor parsing of resources.

2016-10-10 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review152010 --- Fix it, then Ship it! Committing with the following

Re: Review Request 51999: Refactor parsing of resources.

2016-10-05 Thread Jiang Yan Xu
> On Oct. 5, 2016, 11:06 a.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, line 167 > > > > > > "If that fails" gave me the wrong idea of the condition under which the > > method would then call

Re: Review Request 51999: Refactor parsing of resources.

2016-10-05 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review151115 --- Fix it, then Ship it! Overall LGTM!

Re: Review Request 51999: Refactor parsing of resources.

2016-10-05 Thread Jiang Yan Xu
> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, line 199 > > > > > > s/JSON::Array& resourcesJSON/std::string text/ > > > > Looking the current and potential

Re: Review Request 51999: Refactor parsing of resources.

2016-10-03 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Oct. 3, 2016, 11:46 p.m.) Review request for mesos and Jiang Yan Xu.

Re: Review Request 51999: Refactor parsing of resources.

2016-10-03 Thread Anindya Sinha
> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 587 > > > > > > I think it is not good to update here in this patch as I cannot see the > > reason why do we want to update here

Re: Review Request 51999: Refactor parsing of resources.

2016-10-01 Thread Guangya Liu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review150800 --- include/mesos/resources.hpp (line 172)

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha
> On Sept. 27, 2016, 5:35 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 195-196 > > > > > > We should probably add a note below > > > > ``` > > The returned vector of Resource objects

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 28, 2016, 7:24 p.m.) Review request for mesos and Jiang Yan Xu.

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha
> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 478-484 > > > > > > Per our discussion I thought we would delay the validation until when > > we construct `Resources`

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Guangya Liu
> On 九月 27, 2016, 5:28 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 587-617 > > > > > > Is the following cleaner? (I added the logic to validate resources in > > the result vector instead of

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Guangya Liu
> On 九月 22, 2016, 10:14 p.m., Guangya Liu wrote: > > src/common/resources.cpp, line 677 > > > > > > I think that we should still use > > `internal::validateCommandLineResources` as the command line resources > >

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review150581 --- include/mesos/resources.hpp (lines 191 - 192)

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Jiang Yan Xu
> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote: > > src/tests/resources_tests.cpp, lines 630-657 > > > > > > With out refactor, the result of `Resources::parse()` doesn't change > > right? s/out/our/ -

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review150494 --- include/mesos/resources.hpp (lines 56 - 64)

Re: Review Request 51999: Refactor parsing of resources.

2016-09-27 Thread Jiang Yan Xu
> On Sept. 20, 2016, 10 a.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 60-64 > > > > > > We can get rid of this forward declaration if we can get rid of the > > internal convertJSON. > > Anindya

Re: Review Request 51999: Refactor parsing of resources.

2016-09-26 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 26, 2016, 8:58 p.m.) Review request for mesos and Jiang Yan Xu.

Re: Review Request 51999: Refactor parsing of resources.

2016-09-26 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 26, 2016, 6:46 a.m.) Review request for mesos and Jiang Yan Xu.

Re: Review Request 51999: Refactor parsing of resources.

2016-09-26 Thread Anindya Sinha
> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 183-217 > > > > > > I think we do not intend to expose those two APIs, so `private` them? We would call these two APIs from

Re: Review Request 51999: Refactor parsing of resources.

2016-09-26 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review150291 --- include/mesos/resources.hpp (lines 179 - 213)

Re: Review Request 51999: Refactor parsing of resources.

2016-09-22 Thread Guangya Liu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review150081 --- include/mesos/resources.hpp (lines 179 - 213)

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-20 Thread Anindya Sinha
> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 187-188 > > > > > > Why update here, I prefer > > > > ``` > > parses text in the form

Re: Review Request 51999: Refactor parsing of resources.

2016-09-20 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 21, 2016, 4:14 a.m.) Review request for mesos and Jiang Yan Xu.

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-20 Thread Anindya Sinha
> On Sept. 20, 2016, 5 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 60-64 > > > > > > We can get rid of this forward declaration if we can get rid of the > > internal convertJSON. Based on the

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-20 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review149624 --- include/mesos/resources.hpp (lines 60 - 64)

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Guangya Liu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review149603 --- Ditto for v1 include/mesos/resources.hpp (lines 187 - 188)

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Guangya Liu
> On 九月 19, 2016, 6 a.m., Guangya Liu wrote: > > src/slave/containerizer/containerizer.cpp, lines 64-71 > > > > > > A question here: Does the `resources` here will include the zero value > > resource? > > > >

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 19, 2016, 10:42 p.m.) Review request for mesos and Jiang Yan

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Anindya Sinha
> On Sept. 19, 2016, 6 a.m., Guangya Liu wrote: > > src/slave/containerizer/containerizer.cpp, lines 64-71 > > > > > > A question here: Does the `resources` here will include the zero value > > resource? > > >

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Guangya Liu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review149431 --- include/mesos/resources.hpp (line 184)

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-19 Thread Guangya Liu
> On 九月 19, 2016, 6 a.m., Guangya Liu wrote: > > Ditto for all v1 files. - Guangya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/#review149431

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-18 Thread Anindya Sinha
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51999/ --- (Updated Sept. 19, 2016, 5:01 a.m.) Review request for mesos and Jiang Yan Xu.