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:28:15 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.
> 
> Maxim Khutornenko wrote:
>     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?
> 
> Bill Farner wrote:
>     I agree with your thought — the lock on getQuota doesn't add much currently.

Had an offline discussion with Kevin and we came to the same conclusion. There is a current
resource race where different jobs of the same role would draw from the same quota pool. That
race could potentially be resolved by applying a role level lock. However, we would need to
have a hierarchical storage in place first to correctly implement locking anywhere higher
than job level. Dropping it for now. 


- 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