Return-Path: 
 <reviews-return-72-apmail-aurora-reviews-archive=aurora.apache.org@aurora.incubator.apache.org>
X-Original-To: apmail-aurora-reviews-archive@minotaur.apache.org
Delivered-To: apmail-aurora-reviews-archive@minotaur.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 3116C106CE
	for <apmail-aurora-reviews-archive@minotaur.apache.org>;
 Tue, 14 Jan 2014 02:03:24 +0000 (UTC)
Received: (qmail 52668 invoked by uid 500); 14 Jan 2014 02:03:21 -0000
Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org
Received: (qmail 52641 invoked by uid 500); 14 Jan 2014 02:03:20 -0000
Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:reviews-help@aurora.incubator.apache.org>
List-Unsubscribe: <mailto:reviews-unsubscribe@aurora.incubator.apache.org>
List-Post: <mailto:reviews@aurora.incubator.apache.org>
List-Id: <reviews.aurora.incubator.apache.org>
Reply-To: reviews@aurora.incubator.apache.org
Delivered-To: mailing list reviews@aurora.incubator.apache.org
Received: (qmail 52632 invoked by uid 99); 14 Jan 2014 02:03:19 -0000
Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 14 Jan 2014 02:03:19 +0000
X-ASF-Spam-Status: No, hits=-1997.9 required=5.0
	tests=ALL_TRUSTED,HTML_MESSAGE,RP_MATCHES_RCVD
X-Spam-Check-By: apache.org
Received: from [140.211.11.3] (HELO mail.apache.org) (140.211.11.3)
    by apache.org (qpsmtpd/0.29) with SMTP; Tue, 14 Jan 2014 02:03:13 +0000
Received: (qmail 49270 invoked by uid 99); 14 Jan 2014 02:02:44 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 14 Jan 2014 02:02:44 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id 8E8BC1C44C6;
	Tue, 14 Jan 2014 02:02:44 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============9155962399984956600=="
MIME-Version: 1.0
Subject: Re: Review Request 16629: Client quota check (server side)
From: "Maxim Khutornenko" <maxim@apache.org>
To: "Bill Farner" <wfarner@apache.org>, "Kevin Sweeney" <kevints@apache.org>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>
Date: Tue, 14 Jan 2014 02:02:44 -0000
Message-ID: <20140114020244.26519.73881@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
X-ReviewGroup: Aurora
X-ReviewRequest-URL: https://reviews.apache.org/r/16629/
X-Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20140113225813.26519.79233@reviews.apache.org>
In-Reply-To: <20140113225813.26519.79233@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>
X-ReviewRequest-Repository: aurora
X-Virus-Checked: Checked by ClamAV on apache.org

--===============9155962399984956600==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit



> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, line 62
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line62>
> >
> >     This makes the details field kind of lame.  How about Optional<String> to make it obvious that it can be empty?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, line 66
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line66>
> >
> >     checkNotNull, ditto below

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java, line 75
> > <https://reviews.apache.org/r/16629/diff/4/?file=420261#file420261line75>
> >
> >     Convention dictates that accessors start with 'get'.  Please change to getResult() and getDetails().

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java, line 2
> > <https://reviews.apache.org/r/16629/diff/4/?file=420263#file420263line2>
> >
> >     Happy new year!  If you're using a file template in intellij, you can parameterize the year.

Same to you! :) Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 60
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line60>
> >
> >     The doc and signature aren't currently enough to explain what this method does.  i.e. from this context it's not obvious what a "task change" is.  Looks like the missing details are that the check relates to the owner prescribed within 'template', and that the check is to validate whether the role can add 'instanceCount' 'template'-sized tasks.
> >     
> >     Also, s/instanceCount/instances/

Thanks, rephrased.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 89
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line89>
> >
> >     checkNotNull ownerRole

Both args already checked in the SchedulerThriftInterface.setQuota().


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java, line 90
> > <https://reviews.apache.org/r/16629/diff/4/?file=420264#file420264line90>
> >
> >     New behavior, but while you're in here you might as well check these fields more fully (i.e. positive)

I presume we should still allow zeros there to allow partial or total "quota-lock" feature (i.e. when one or more specs are set to zero to prevent any additions/mutations)? Added ">=0" validation.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/quota/Quotas.java, line 32
> > <https://reviews.apache.org/r/16629/diff/4/?file=420266#file420266line32>
> >
> >     It's good to see some of these methods disappear.  It would be great to see the visibility of more of these methods reduced, or moved if there's only one caller.  This would further establish that the meaning of quota is owned by QuotaManager.  Please take a quick pass to see if any of these can be removed, moved, or reduce visibility.

Reduced and moved :) I am hesitant to completely drop this class as Quotas.noQuota() would still read better than QuotaManager.noQuota().


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, line 131
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line131>
> >
> >     This comment might lose context quickly after this review is closed.  Consider instead commenting above validateTaskLimits, noting that it's performed inside the transaction to avoid a data race.

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, line 144
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line144>
> >
> >     Can you leave a TODO for me to remove the JobManager abstraction, and directly invoke addInstances here for non-cron jobs?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, line 167
> > <https://reviews.apache.org/r/16629/diff/4/?file=420268#file420268line167>
> >
> >     s/count/instances/ to be consistent with elsewhere?

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 623
> > <https://reviews.apache.org/r/16629/diff/4/?file=420271#file420271line623>
> >
> >     i find the flow easier to follow if it were right after saveQuota in the try{}.

Done.


> On Jan. 13, 2014, 10:58 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java, line 164
> > <https://reviews.apache.org/r/16629/diff/4/?file=420275#file420275line164>
> >
> >     Isn't it easier to just construct a fake value here, and avoid the stub?  I would find that easier to follow, anyhow, given that the class is purely a container.

That would require bumping up the constructor access level that I would rather not.


- Maxim


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


On Jan. 10, 2014, 9:23 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2014, 9:23 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Part 2: Server side changes for the client quota check. 
> 
> Refactored quota manager:
> - Merged QuotaFilter with QuotaManager and dropped JobFilter implementation;
> - Simplified quota manager logic by splitting data retrieval and quota checking steps;
> - Moved quota checks into write transaction to ensure consistency.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java cef0ff28bb0c0e08c5efaa1ed326f66bc9ffa5d9 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaComparisonResult.java 99d2e4c72621708c971d25ad4e6722e0870093af 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaFilter.java 6ab79820a0634478c0525d7fdd5a4d002ef8ea08 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 6b0645ba93e50b576f7e572d8dc06231636fade2 
>   src/main/java/org/apache/aurora/scheduler/quota/QuotaModule.java 4a619492f6e9eb41e693353187fc3b1781bffc1f 
>   src/main/java/org/apache/aurora/scheduler/quota/Quotas.java 24f209339f3a6f4659693986e220187bd34d2fb5 
>   src/main/java/org/apache/aurora/scheduler/state/JobFilter.java 0d84c1e2eff781e7d0250967ae6b9f9473fde3dc 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 1d450f2d2d8e747878b67bccbf3fd7d018a52d20 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 
>   src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5 
>   src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaComparisonResultTest.java 23069b8d191f1675636bceb8c297ebcc0d88d8dc 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaFilterTest.java b1d878ea91c02ba87059b05877208b702d3fbcae 
>   src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java f971aa1882e5e9f4208d177566779f5dd12d70ce 
>   src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 720d0c86d8b112bf92196cbb81ece44476534654 
>   src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 91c1c24448092e1b3454844ab8074ed030383594 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java cce27a0e37452f370a3729b6b05bf0bea29f85f6 
> 
> Diff: https://reviews.apache.org/r/16629/diff/
> 
> 
> Testing
> -------
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


--===============9155962399984956600==--