> 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?
I agree with your thought — the lock on getQuota doesn't add much currently. - Bill ----------------------------------------------------------- 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 > >
