Re: Review Request 49910: Output disk resource source information.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On July 12, 2016, 6:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> 
> Diff: https://reviews.apache.org/r/49910/diff/1/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 49910: Output disk resource source information.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49910]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 12, 2016, 6:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 49910: Output disk resource source information.

[Alexander Rojas]

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



Hey Tim. Beyond AlexR suggestions, the patch looks good to me. However, you 
need to remember the description will become the commit message and therefore 
we prefer to avoid repeating the summary in the description. How about 
something along the lines:

> This patch addresses the generation of confusing error 
> messages when requested disk resources do not include 
> any source information and therefore end up being treated 
> as root volumes.
>
> Compare the generated old error message:
>
> ```
> Task uses more resources
> cpus(*):4; mem(*):4096; ports(*):[31000-31000]; disk(kafka, 
> kafka)[kafka_0:data]:960679
> than available
> cpus(*):32; mem(*):256819;  ports(*):[31000-32000]; disk(kafka, 
> kafka)[kafka_0:data]:960679;   disk(*):240169;
> ```
>
> With new:
>
> _Your example here_

- Alexander Rojas


On July 12, 2016, 8:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 49910: Output disk resource source information.

[Alexander Rojas]

> On July 12, 2016, 8:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 1486-1488
> > 
> >
> > Let's add the default case as well (see e.g. how `Resource` is printed).

Not sure adding a default case is the way to go. At least lately we have move 
away from it. The reason being, if we add more elements to the ENUM and there 
is a default case, we wont get neither a warning nor a compulation error if it 
is not handled in every `switch`.


- Alexander


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


On July 12, 2016, 8:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 49910: Output disk resource source information.

[Tim Harper]

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

(Updated July 12, 2016, 6:14 a.m.)


Review request for mesos, Alexander Rukletsov and Alexander Rojas.


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


Repository: mesos


Description
---

Output disk resource source information.


Diffs
-

  src/common/resources.cpp cf07072 

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


Testing
---

I ran `make check` in my OS X build environment. I had to disable SVN tests 
because they didn't work, but I doubt this affected that feature set.

Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)


Thanks,

Tim Harper

Review Request 49910: Output disk resource source information.

[Tim Harper]

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

Review request for mesos, Alexander Rukletsov and Alexander Rojas.


Repository: mesos


Description
---

Output disk resource source information.


Diffs
-

  src/common/resources.cpp cf07072 

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


Testing
---

I ran `make check` in my OS X build environment. I had to disable SVN tests 
because they didn't work, but I doubt this affected that feature set.

Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)


Thanks,

Tim Harper