aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 16629: Client quota check (server side)
Date Sat, 04 Jan 2014 01:13:51 GMT


> On Jan. 4, 2014, 12:55 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/LockManager.java, line 65
> > <https://reviews.apache.org/r/16629/diff/1/?file=415149#file415149line65>
> >
> >     Is there a reason to take an Optional here? Shouldn't the caller just not call
in if the provided lock is null?
> 
> Maxim Khutornenko wrote:
>     This is a convenience method for cases like getQuota() where Lock presence is optional
as it's used in both locked (update, create) and unlocked (getQuota api) contexts. Having
Optional here avoids IF condition on the SchedulerThriftInterface side.
> 
> Kevin Sweeney wrote:
>     I personally prefer that input sanitization happen closer to the request processing
code when possible (which allows us to assume that no nulls are passed to core objects) but
I'll defer to consensus here.

Actually, it might even make sense to drop the Lock from getQuota() call altogether. The getQuota()
does not make any changes to the system and both update and create flows would already have
locks acquired by the time this RPC is called. So, validating lock context within getQuota()
would not add anything. Any objections? 


- Maxim


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


On Jan. 4, 2014, 12:03 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16629/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2014, 12:03 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Part 2: Server side changes for the client quota check. 
> 
> Note: api.thrift changes are duplicated from part 1: https://reviews.apache.org/r/16444/
and will race to submission with it.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/state/LockManager.java 5f34de1bd0f90269642ea8d451ce4cd6fe180c2d

>   src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java bf0a650ae55eaa66ae7ee2b8a201cb5bda38177f

>   src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 8fb51d69be6d370f9f010c797b2c1205b38a04f5

>   src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java c1a11bdb91c5e764864324d26248d1783af8048b

>   src/main/thrift/org/apache/aurora/gen/api.thrift 480b8f472bcfbe547a91c41638250350a0110334

>   src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e

>   src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
91c1c24448092e1b3454844ab8074ed030383594 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 62fc8045f6a5fda234df73452685bd04e3142aaf

> 
> Diff: https://reviews.apache.org/r/16629/diff/
> 
> 
> Testing
> -------
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


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