Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-05-03 Thread Jiang Yan Xu 

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


Ship it!




Committing with minor edits.


src/master/master.cpp
Line 8901 (original), 9083 (patched)


s/executors/frameworks/ 

This map is keyed by frameworks.



src/master/master.cpp
Line 8950 (original), 9132 (patched)


Ditto.


- Jiang Yan Xu


On May 2, 2017, 10:53 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated May 2, 2017, 10:53 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-05-02 Thread Anindya Sinha 

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

(Updated May 2, 2017, 5:53 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The following metrics take the shared counts of shared resources into
account in determination of the actual amount of used resources:
a) `master/_used`
b) `master/_revocable_used`
c) `slave/_used`
d) `slave/_revocable_used`


Diffs (updated)
-

  src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
  src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 


Diff: https://reviews.apache.org/r/57963/diff/4/

Changes: https://reviews.apache.org/r/57963/diff/3-4/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-05-01 Thread Jiang Yan Xu 

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


Fix it, then Ship it!





src/master/master.cpp
Lines 8950 (patched)


s/same/the same/ I forgot to say "all ocurrences".



src/slave/slave.cpp
Line 6743 (original), 6743 (patched)


Perhaps I am pedantic but I think s/used/Resources arithmetic/ here is more 
clear. When I read "We use `used`" my immediate reaction was "instead of what?" 
because `used` is the only variable here. The comment is basically to remind 
the reader of the "magic" of Resources::+= so perhaps just directly say it?

We could use the same wording for the `slaveUsed` case above so all of 
these comments are consistent (which was my original suggestion).


- Jiang Yan Xu


On April 22, 2017, 10 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated April 22, 2017, 10 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
>   src/slave/slave.cpp 4ff522e75bc8de34fe2e7720bdd8ce3d32cbf803 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-04-22 Thread Anindya Sinha 

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

(Updated April 22, 2017, 5 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

The following metrics take the shared counts of shared resources into
account in determination of the actual amount of used resources:
a) `master/_used`
b) `master/_revocable_used`
c) `slave/_used`
d) `slave/_revocable_used`


Diffs (updated)
-

  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
  src/slave/slave.cpp 4ff522e75bc8de34fe2e7720bdd8ce3d32cbf803 


Diff: https://reviews.apache.org/r/57963/diff/3/

Changes: https://reviews.apache.org/r/57963/diff/2-3/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-04-21 Thread Jiang Yan Xu 


> On April 21, 2017, 10:37 a.m., Jiang Yan Xu wrote:
> > LGTM!
> > 
> > Can we have a test? It doesn't have to be a standalone test if we already 
> > have a test that just launches tasks from two frameworks, then we can just 
> > add it to the test? If not seems like that's proper test to have in general?

Nevermind the test comment, now I see there's a follow-up patch for it.


- Jiang Yan


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


On March 27, 2017, 3:27 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated March 27, 2017, 3:27 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-04-21 Thread Jiang Yan Xu 

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



LGTM!

Can we have a test? It doesn't have to be a standalone test if we already have 
a test that just launches tasks from two frameworks, then we can just add it to 
the test? If not seems like that's proper test to have in general?


src/master/master.cpp
Lines 8896 (patched)


s/same/the same/?



src/slave/slave.cpp
Lines 6703 (patched)


The `slave` in `slaveUsed` seems redundant here (my example was on the 
master), here we have only one agent.

s/slaveUsed/used/?



src/slave/slave.cpp
Lines 6747 (patched)


Ditto.


- Jiang Yan Xu


On March 27, 2017, 3:27 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated March 27, 2017, 3:27 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-03-27 Thread Anindya Sinha 


> On March 27, 2017, 7:43 p.m., Jiang Yan Xu wrote:
> > src/master/master.cpp
> > Lines 8894-8900 (original), 8894-8911 (patched)
> > 
> >
> > Something like this more elegant?
> > 
> > ```
> > double Master::_resources_used(const string& name)
> > {
> >   double used = 0.0;
> > 
> >   foreachvalue (Slave* slave, slaves.registered) {
> > // We need to use `Resource` instead of `double` here for its
> > // `+=` operator that takes care of de-duplicating the same shared
> > // resources across frameworks.
> > Resources slaveUsed; 
> > foreachvalue (const Resources& resources, slave->usedResources) {
> >   slaveUsed += resources.nonRevocable();
> > }
> > used +=
> >   
> > slaveUsed.get(name).getOrElse(Value::Scalar()).value();
> >   }
> > 
> >   return used;
> > }
> > ```
> > 
> > Here and below.

Sure. Updated.


- Anindya


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


On March 27, 2017, 10:27 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated March 27, 2017, 10:27 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-03-27 Thread Anindya Sinha 

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

(Updated March 27, 2017, 10:27 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The following metrics take the shared counts of shared resources into
account in determination of the actual amount of used resources:
a) `master/_used`
b) `master/_revocable_used`
c) `slave/_used`
d) `slave/_revocable_used`


Diffs (updated)
-

  src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
  src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 


Diff: https://reviews.apache.org/r/57963/diff/2/

Changes: https://reviews.apache.org/r/57963/diff/1-2/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-03-27 Thread Jiang Yan Xu 

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




src/master/master.cpp
Lines 8894-8900 (original), 8894-8911 (patched)


Something like this more elegant?

```
double Master::_resources_used(const string& name)
{
  double used = 0.0;

  foreachvalue (Slave* slave, slaves.registered) {
// We need to use `Resource` instead of `double` here for its
// `+=` operator that takes care of de-duplicating the same shared
// resources across frameworks.
Resources slaveUsed; 
foreachvalue (const Resources& resources, slave->usedResources) {
  slaveUsed += resources.nonRevocable();
}
used +=
  slaveUsed.get(name).getOrElse(Value::Scalar()).value();
  }

  return used;
}
```

Here and below.


- Jiang Yan Xu


On March 27, 2017, 11:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated March 27, 2017, 11:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 57963: Metrics for used resources should incorporate shared resources.

2017-03-27 Thread Anindya Sinha 

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

The following metrics take the shared counts of shared resources into
account in determination of the actual amount of used resources:
a) `master/_used`
b) `master/_revocable_used`
c) `slave/_used`
d) `slave/_revocable_used`


Diffs
-

  src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
  src/slave/slave.cpp c8479d7e8eb915284f0ea8cf75f47acd679dee7e 


Diff: https://reviews.apache.org/r/57963/diff/1/


Testing
---

All tests passed.


Thanks,

Anindya Sinha