Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 20, 2016, 9:34 p.m., Jiang Yan Xu wrote:
> > Given these comments, do you want to punt on improving the simple string 
> > parsing and get the JSON format working first?
> > 
> > Also we should have independent unit tests here in resources_tests.cpp, 
> > this shouldn't to be coupled with the containerizer.

Ok. I will close this one for now.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149710
---


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 613
> > 
> >
> > Not yours, but I prefer that we quota token as 
> > 
> > ```
> > "Bad value for resources, missing or extra ':' in '" + token + "'");
> > ```

Both patterns are in use. I suggest if we want to make that consistent, we do 
it separately.


> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 633-634
> > 
> >
> > I prefer that we quota `key` and `token`
> > 
> > ```
> > return Error(
> > "Bad value for resources, mismatched brackets in '" +
> >  key + "' for '" + token + "'");
> > ```
> > 
> > Ditto for others

Same comment as before.


> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 700
> > 
> >
> > What about adding `UNREACHABLE()` here?

`Resource::DiskInfo::Source::Type_Parse()` returns false if it does not result 
in a valid `Resource::DiskInfo::Source::Type` and in that case, we return an 
error. So, at this point, we type should contain valid values.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149611
---


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149611
---




src/common/resources.cpp (line 613)


Not yours, but I prefer that we quota token as 

```
"Bad value for resources, missing or extra ':' in '" + token + "'");
```



src/common/resources.cpp (lines 633 - 634)


I prefer that we quota `key` and `token`

```
return Error(
"Bad value for resources, mismatched brackets in '" +
 key + "' for '" + token + "'");
```

Ditto for others



src/common/resources.cpp (line 700)


What about adding `UNREACHABLE()` here?


- Guangya Liu


On 九月 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated 九月 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[haosdent huang]

> On Sept. 19, 2016, 4:55 p.m., haosdent huang wrote:
> > src/common/resources.cpp, lines 616-648
> > 
> >
> > This is conflict with range resources `ports:[21000-24000,3-34000]`.
> 
> Anindya Sinha wrote:
> I do not think so. `ports:[21000-24000,3-34000]` will have 
> `key=ports` and `value=[21000-24000,3-34000]`. So this additional loop 
> would not be entered since the `key` does not include the square brackets. 
> Note that key is set based on number of tokens (2 in this specific case, and 
> 3 when we include MOUNT/PATH disks, ie. `disk[MOUNT:/tmp]:2048`). The new 
> logic is not applied on value, but only on key (which is whole resource 
> excluding the value).

Got it, thanks for your explanation! Another question is if we have more 
attributes in the future, would it use this form?

```
disk[attr_1:value_1,attr_2:value2]
```


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149490
---


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 19, 2016, 4:55 p.m., haosdent huang wrote:
> > src/common/resources.cpp, lines 616-648
> > 
> >
> > This is conflict with range resources `ports:[21000-24000,3-34000]`.

I do not think so. `ports:[21000-24000,3-34000]` will have `key=ports` and 
`value=[21000-24000,3-34000]`. So this additional loop would not be entered 
since the `key` does not include the square brackets. Note that key is set 
based on number of tokens (2 in this specific case, and 3 when we include 
MOUNT/PATH disks, ie. `disk[MOUNT:/tmp]:2048`). The new logic is not applied on 
value, but only on key (which is whole resource excluding the value).


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149490
---


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 19, 2016, 6:40 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 611
> > 
> >
> > A unit test is needed to test the case when token size is 3

Tests for various cases of token sizes (2 and 3) are captured in 
https://reviews.apache.org/r/51880/


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149433
---


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/
---

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs (updated)
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

Diff: https://reviews.apache.org/r/52001/diff/


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149490
---




src/common/resources.cpp (lines 616 - 648)


This is conflict with range resources `ports:[21000-24000,3-34000]`.



src/common/resources.cpp (line 617)


use `back()` instead of `[xx.size() - 1]`



src/common/resources.cpp (lines 680 - 693)


Use `Resource::DiskInfo::Source::Type_Parse` instead.


- haosdent huang


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/#review149433
---



Ditto for v1


src/common/resources.cpp (line 610)


s/pair/tokens

pair is not good here, I intend to understand it has two values.



src/common/resources.cpp (line 611)


A unit test is needed to test the case when token size is 3



src/common/resources.cpp (lines 626 - 627)


new line here



src/common/resources.cpp (line 632)


What about print out the `key` but not `token` here as the `key` might be 
more accurate.



src/common/resources.cpp (line 642)


How about s/token/diskTypeRoot



src/common/resources.cpp (lines 682 - 683)


new line here



src/common/resources.cpp (lines 687 - 688)


new line here


- Guangya Liu


On 九月 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated 九月 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/
---

(Updated Sept. 19, 2016, 5:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

Diff: https://reviews.apache.org/r/52001/diff/


Testing
---


Thanks,

Anindya Sinha

Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52001/
---

Review request for mesos.


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

Diff: https://reviews.apache.org/r/52001/diff/


Testing
---


Thanks,

Anindya Sinha