Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-11-03 Thread Maxim Khutornenko 

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

(Updated Nov. 3, 2014, 6:52 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-825
https://issues.apache.org/jira/browse/AURORA-825


Repository: aurora


Description
---

Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). This 
is rolled up into overall role consumption for reporting and quota checks.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
  src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
3fbab8b27403a416e86c97970eb6124c8a34c38c 
  src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
2fe257416e8fc11be3ebf0dabbf4f63628b3c670 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-11-03 Thread Maxim Khutornenko 


> On Nov. 3, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java, line 16
> > 
> >
> > java.util.Objects

Done.


> On Nov. 3, 2014, 6:38 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java, line 87
> > 
> >
> > You'll need to fully-qualify this until we're on Guava 18, which 
> > renames this class to MoreObjects.
> > 
> > ```java
> > return com.google.common.base.Objects.toStringHelper(this)
> >...
> >.toString();
> > ```

Done.


- Maxim


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


On Oct. 30, 2014, 5:11 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 30, 2014, 5:11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-11-03 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On Oct. 30, 2014, 5:11 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 30, 2014, 5:11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-11-03 Thread Kevin Sweeney 

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

Ship it!



src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java


java.util.Objects



src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java


You'll need to fully-qualify this until we're on Guava 18, which renames 
this class to MoreObjects.

```java
return com.google.common.base.Objects.toStringHelper(this)
   ...
   .toString();
```


- Kevin Sweeney


On Oct. 30, 2014, 10:11 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 30, 2014, 10:11 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-11-03 Thread Maxim Khutornenko 

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


Ping.

- Maxim Khutornenko


On Oct. 30, 2014, 5:11 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 30, 2014, 5:11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-10-30 Thread Aurora ReviewBot 

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


Master (d85e616) is red with this patch.
  ./build-support/jenkins/build.sh


Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter.common.string-0.3.1-py2.7-nspkg.pth
  Running setup.py install for twitter.common.threading
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)

Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter.common.threading-0.3.1-py2.7-nspkg.pth
  Running setup.py install for twitter.common.util
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)

Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/lib/python2.7/site-packages/twitter.common.util-0.3.1-py2.7-nspkg.pth
  Found existing installation: setuptools 3.6
Uninstalling setuptools:
  Successfully uninstalled setuptools
Successfully installed Markdown Pygments ansicolors cov-core coverage lockfile 
pantsbuild.pants pex psutil py pystache pytest pytest-cov python-daemon 
requests twitter.common.collections twitter.common.config 
twitter.common.confluence twitter.common.contextutil twitter.common.decorators 
twitter.common.dirutil twitter.common.lang twitter.common.log 
twitter.common.options twitter.common.process twitter.common.string 
twitter.common.threading twitter.common.util setuptools
Cleaning up...

Exception message: Failed to fetch 
https://pypi.python.org/packages/source/s/setuptools/setuptools-2.2.tar.gz#md5=04a7664538957b832710653fd7d5b4e6
 to /tmp/user/10021/tmphiCoYf: Could not reach 
https://pypi.python.org/packages/source/s/setuptools/setuptools-2.2.tar.gz#md5=04a7664538957b832710653fd7d5b4e6
 within deadline.

- Aurora ReviewBot


On Oct. 30, 2014, 5:11 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 30, 2014, 5:11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-10-30 Thread Maxim Khutornenko 

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

(Updated Oct. 30, 2014, 5:11 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-825
https://issues.apache.org/jira/browse/AURORA-825


Repository: aurora


Description
---

Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). This 
is rolled up into overall role consumption for reporting and quota checks.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
  src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
3fbab8b27403a416e86c97970eb6124c8a34c38c 
  src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
2fe257416e8fc11be3ebf0dabbf4f63628b3c670 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-10-30 Thread Maxim Khutornenko 


> On Oct. 30, 2014, 4:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 420
> > 
> >
> > Is there a reason we don't use `Resources` here?  Seems like you could 
> > leverage it for the ITaskConfig->Resources translation and addition, then 
> > you're only in need of the Resources->IResourceAggregate step.
> > 
> > AFAICT you'll get to remove some private helpers from this class, and 
> > this loop becomes more concise:
> > ```java
> > Resources sum = Resources.NONE;  // Need to create this
> > for (..) {
> >   
> >   sum = Resources.sum(sum, Resources.from(task));
> > }
> > ```

`Resources` does not feel appropriate here as it also deals with ports, which 
is compeltely irrelevant. Since we have to work with IResourceAggregate here 
anyway, bringing yet another object with a similar but slightly different 
structure will just add more confusion the way I see it.

BTW, this particular loop is not a simple sum but rather a sum of tasks scaled 
out to instances. It can also be acoomplished with `IResourceAggregate` as:
```java
IResourceAggregate sum = ResourceAggregates.EMPTY;
for(...) {
  sum = add(sum, ResourceAggregates.scale(fromTasks(ImmutableSet.of(task)), 
instances);
}
```

The reason I did not do it this way is just to reduce unnecessary object 
creation churn. Happy to rework it though as updates should never have the 
number of task configs high enough to make any sensible perf impact.


> On Oct. 30, 2014, 4:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 208
> > 
> >
> > AFAICT you know at this point whether `requestedUpdate` needs to be 
> > added to `updatesByKey`.  Can you do it here rather than two levels down 
> > the stack, and sae the parameter?

Great suggestion. Done.


> On Oct. 30, 2014, 4:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 205
> > 
> >
> > Line break style doesn't match project convention (we try to split up 
> > the chained call).  How about this:
> > 
> > Map updatesByKey = Maps.newHashMap(
> > fetchActiveJobUpdates(storeProvider.getJobUpdateStore(), role)
> > .uniqueIndex(UPDATE_TO_JOB_KEY));

Done.


- Maxim


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


On Oct. 29, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 29, 2014, 12:20 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-10-30 Thread Bill Farner 

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


Please excuse me if this seems like nit-picking, i'm just searching for ways to 
simplify this code.  It's already conceptually complicated, i'm just hoping we 
can make the code as simple as it can be.


src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java


Line break style doesn't match project convention (we try to split up the 
chained call).  How about this:

Map updatesByKey = Maps.newHashMap(
fetchActiveJobUpdates(storeProvider.getJobUpdateStore(), role)
.uniqueIndex(UPDATE_TO_JOB_KEY));



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java


AFAICT you know at this point whether `requestedUpdate` needs to be added 
to `updatesByKey`.  Can you do it here rather than two levels down the stack, 
and sae the parameter?



src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java


Is there a reason we don't use `Resources` here?  Seems like you could 
leverage it for the ITaskConfig->Resources translation and addition, then 
you're only in need of the Resources->IResourceAggregate step.

AFAICT you'll get to remove some private helpers from this class, and this 
loop becomes more concise:
```java
Resources sum = Resources.NONE;  // Need to create this
for (..) {
  
  sum = Resources.sum(sum, Resources.from(task));
}
```


- Bill Farner


On Oct. 29, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 29, 2014, 12:20 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 27317: Adding resource consumption calculation for cron jobs.

2014-10-28 Thread Aurora ReviewBot 

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


Master (978998a) is red with this patch.
  ./build-support/jenkins/build.sh

  pants build (options) [spec] (build args)
  pants build (options) [spec]... -- (build args)

Options:
  --version show program's version number and exit
  --no-pantsrc  Specifies that pantsrc files should be ignored.
  --log-exitLog an exit message on success or failure.
  -t CONN_TIMEOUT, --timeout=CONN_TIMEOUT
Number of seconds to wait for http connections.
  -i INTERPRETERS, --interpreter=INTERPRETERS
Constrain what Python interpreters to use.  Uses
Requirement format from pkg_resources, e.g.
'CPython>=2.6,<3' or 'PyPy'. By default, no
constraints are used.  Multiple constraints may be
added.  They will be ORed together.
  -v, --verbose Show verbose output.
  -f, --fastRun tests in a single chroot.

Builds the specified Python target(s). Use ./pants goal for JVM and other
targets.

- Aurora ReviewBot


On Oct. 29, 2014, 12:20 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27317/
> ---
> 
> (Updated Oct. 29, 2014, 12:20 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-825
> https://issues.apache.org/jira/browse/AURORA-825
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Cron job consumption is now calculated as MAX(SUM(template), SUM(tasks)). 
> This is rolled up into overall role consumption for reporting and quota 
> checks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java 
> d4e0f5335c9ebd8da97cb9634152b46c2bd9affe 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
> 3fbab8b27403a416e86c97970eb6124c8a34c38c 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 
> 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 
> 
> Diff: https://reviews.apache.org/r/27317/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>