aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 27317: Adding resource consumption calculation for cron jobs.
Date Thu, 30 Oct 2014 17:11:11 GMT


> On Oct. 30, 2014, 4:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 420
> > <https://reviews.apache.org/r/27317/diff/1/?file=736703#file736703line420>
> >
> >     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
> > <https://reviews.apache.org/r/27317/diff/1/?file=736703#file736703line208>
> >
> >     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
> > <https://reviews.apache.org/r/27317/diff/1/?file=736703#file736703line205>
> >
> >     Line break style doesn't match project convention (we try to split up the chained
call).  How about this:
> >     
> >         Map<IJobKey, IJobUpdate> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message