Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 9, 2015, 4:48 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 9, 2015, 6:43 a.m., Adam B wrote: > > src/common/resources.cpp, lines 356-357 > > > > > > Doesn't Resources::validate() return an Error? Why not use that Error > > and its message with your return?

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 6, 2015, 10:13 a.m., Greg Mann wrote: > > src/tests/resources_tests.cpp, lines 593-613 > > > > > > I fixed an unintended error in this jsonString, and then found that the > > parsing didn't return an

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105607 --- Almost there, but I won't let you "silently ignore" any errors on

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 8, 2015, 9:32 a.m., Adam B wrote: > > src/common/resources.cpp, line 271 > > > > > > Red flag: "silently ignored" scares me. Don't you mean that an error in > > validate() will yield an empty

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 9, 2015, 5:57 a.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105653 --- Ship it! Much better error handling, but we're still dropping an

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 7, 2015, 12:17 a.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 5, 2015, 1:33 p.m., Adam B wrote: > > src/common/resources.cpp, lines 471-472 > > > > > > Why wrap here? Wasn't this previously 80 chars exactly? It was 80 previously, but now we have 2 additional indent

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 6, 2015, 6:08 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105486 --- src/common/resources.cpp (lines 268 - 271)

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 6, 2015, 6:13 p.m., Greg Mann wrote: > > Note: I raised the following issues just to call attention to these changes made in the patch. I think the code is ready to merge, but wanted to make sure these differences didn't go unnoticed. - Greg

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105234 --- Ship it! Looks great! Just a couple of nits and test comment

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105208 --- Ship it! Ship It! - Guangya Liu On 十一月 4, 2015, 4:21 p.m.,

Re: Review Request 39018: Added JSON parsing for Resources.

> On 十一月 4, 2015, 2:54 a.m., Guangya Liu wrote: > > src/v1/resources.cpp, line 1190 > > > > > > remove this > > Greg Mann wrote: > Rather than removing it, I thought it might be nice to expand this > comment

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 4, 2015, 3:31 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 4, 2015, 4:21 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Nov. 4, 2015, 2:54 a.m., Guangya Liu wrote: > > src/v1/resources.cpp, line 1190 > > > > > > remove this Rather than removing it, I thought it might be nice to expand this comment to make it more useful. Let

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review104985 --- Ship it! Ship It! - Michael Park On Oct. 20, 2015, 6:09 a.m.,

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Nov. 3, 2015, 10:55 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 18, 2015, 12:41 a.m., Michael Park wrote: > > src/common/resources.cpp, line 377 > > > > > > I think you need to check for `resource.role() == "*"` instead, since > > `role` has `[default = "*"]`. Please

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105030 --- src/v1/resources.cpp (line 1144)

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 18, 2015, 6:18 p.m., Michael Park wrote: > > src/common/resources.cpp, lines 482-532 > > > > > > How about a structure like the following? > > > > ```cpp > > Resources result; > > > >

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 20, 2015, 6:09 a.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review103216 --- Ship it! Thanks for the update! - Guangya Liu On Oct. 20,

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 18, 2015, 12:41 a.m., Michael Park wrote: > > src/common/resources.cpp, line 377 > > > > > > I think you need to check for `resource.role() == "*"` instead, since > > `role` has `[default = "*"]`. Please

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 18, 2015, 6:18 p.m., Michael Park wrote: > > src/common/resources.cpp, lines 482-532 > > > > > > How about a structure like the following? > > > > ```cpp > > Resources result; > > > >

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review103071 --- include/mesos/resources.hpp (line 37)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review103063 --- src/common/resources.cpp (lines 482 - 530)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review103018 --- src/common/resources.cpp (line 274)

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 114-118 > > > > > > Can this be removed or merged to the under comments? > > Greg Mann wrote: > I put the comment here

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 14, 2015, 3:50 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5 p.m., Michael Park wrote: > > src/common/resources.cpp, line 1198 > > > > > > Why the move here? > > Greg Mann wrote: > I was moving all of the private methods into an

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102664 --- Patch looks great! Reviews applied: [39211, 39018] All tests

Re: Review Request 39018: Added JSON parsing for Resources.

> On 十月 13, 2015, 3:35 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 114-118 > > > > > > Can this be removed or merged to the under comments? > > Greg Mann wrote: > I put the comment here so

Re: Review Request 39018: Added JSON parsing for Resources.

> On 十月 13, 2015, 5:38 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 367 > > > > > > Why not check error here? > > Greg Mann wrote: > I'm sorry, I don't understand this comment. Are you referring

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102584 --- include/mesos/resources.hpp (line 375)

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5 p.m., Michael Park wrote: > > src/common/resources.cpp, line 361 > > > > > > I think it would be better to only support one way of doing things > > here. I would opt for the array format. Is

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 114-118 > > > > > > Can this be removed or merged to the under comments? > > Greg Mann wrote: > I put the comment here

Re: Review Request 39018: Added JSON parsing for Resources.

> On 十月 13, 2015, 3:35 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 114-118 > > > > > > Can this be removed or merged to the under comments? > > Greg Mann wrote: > I put the comment here so

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5 p.m., Michael Park wrote: > > src/common/resources.cpp, line 361 > > > > > > I think it would be better to only support one way of doing things > > here. I would opt for the array format. Is

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102443 --- The documentation added in this patch is great! Also consider

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 13, 2015, 7:34 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102512 --- Patch looks great! Reviews applied: [39211, 39018] All tests

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5 p.m., Michael Park wrote: > > src/common/resources.cpp, line 361 > > > > > > I think it would be better to only support one way of doing things > > here. I would opt for the array format. Is

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 114-118 > > > > > > Can this be removed or merged to the under comments? I put the comment here so that it wouldn't

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 13, 2015, 6:30 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5:38 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 367 > > > > > > Why not check error here? I'm sorry, I don't understand this comment. Are you referring to the error checking

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5:38 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 367 > > > > > > Why not check error here? > > Greg Mann wrote: > I'm sorry, I don't understand this comment. Are you

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 13, 2015, 5 p.m., Michael Park wrote: > > src/common/resources.cpp, line 361 > > > > > > I think it would be better to only support one way of doing things > > here. I would opt for the array format. Is

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102573 --- Patch looks great! Reviews applied: [39211, 39018] All tests

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102203 --- include/mesos/resources.hpp (line 312)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 12, 2015, 6:24 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102294 --- Patch looks great! Reviews applied: [39211, 39018] All tests

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 12, 2015, 7:48 a.m., Guangya Liu wrote: > > include/mesos/resources.hpp, line 312 > > > > > > Can you please make those comments using > >

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 12, 2015, 6:34 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 12, 2015, 6:38 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102382 --- include/mesos/resources.hpp (line 75)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102391 --- src/common/resources.cpp (line 367)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102176 --- Patch looks great! Reviews applied: [39018] All tests passed. -

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102183 --- Patch looks great! Reviews applied: [39211, 39018] All tests

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 11, 2015, 3:08 a.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 9, 2015, 11:14 p.m., Adam B wrote: > > include/mesos/resources.hpp, lines 83-98 > > > > > > Do these all need to be public? Indeed they do not. In addition to making these private, I rearranged the member

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 10, 2015, 12:43 a.m.) Review request for mesos, Adam B,

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 9, 2015, 4:14 p.m., Adam B wrote: > > src/common/resources.cpp, lines 368-384 > > > > > > This smells like a hack to workaround a bug in picojson. Can you link > > to something that indicates this as best

Re: Review Request 39018: Added JSON parsing for Resources.

--- 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

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review102151 --- Patch looks great! Reviews applied: [39018] All tests passed. -

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101871 --- Ship it! Ship It! - Guangya Liu On 十月 7, 2015, 3:24 p.m., Greg

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 7, 2015, 3:24 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 7, 2015, 3:44 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 354 > > > > > > What does this mean? In my understanding, the resource falg should > > support both string and json format, why

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101797 --- Ship it! Ship It! - haosdent huang On Oct. 7, 2015, 3:24 p.m.,

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101740 --- src/common/resources.cpp (line 354)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101616 --- src/common/resources.cpp (line 362)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101739 --- Patch looks great! Reviews applied: [39018] All tests passed. -

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 6, 2015, 11:02 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

> On Oct. 6, 2015, 7:09 a.m., haosdent huang wrote: > > src/common/resources.cpp, line 362 > > > > > > Is it would more clear to split three functions, one is for parse old > > form, one is for parse json object

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101589 --- include/mesos/resources.hpp (line 73)

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 5, 2015, 9:38 p.m.) Review request for mesos, Adam B, Alexander

Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/ --- (Updated Oct. 5, 2015, 9:51 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 39018: Added JSON parsing for Resources.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review101590 --- Patch looks great! Reviews applied: [39018] All tests passed. -