> 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 > >
