----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24521/#review50348 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java <https://reviews.apache.org/r/24521/#comment88039> As we discussed offline - let's remove this and require the caller to follow up with saveJobUpdateEvent when they want to. If the caller neglects to do this, i think it's fine for the subsequent selects to have trouble (for now, at least). src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java <https://reviews.apache.org/r/24521/#comment88040> instanceOverrides.isEmpty() rather than Iterables.isEmpty src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java <https://reviews.apache.org/r/24521/#comment88042> Should be able to revert this since DBJobUpdateStore will no longer require a clock src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java <https://reviews.apache.org/r/24521/#comment88045> Nice. Suggested rewording: "MyBatis returns auto-generated IDs through mutable fields in parameters. This class can be used as an additional {@link org.apache.ibatis.annotations.Param Param} to retrieve the ID when the inserted object is not self-identifying." src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java <https://reviews.apache.org/r/24521/#comment88043> remove extra newline src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88046> {@code false}, {@code true} src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88047> "Container for auto-generated ID of the inserted job update row." src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88048> Can this be primitive boolean instead? src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88050> "Instance ID ranges to associate with a task configuration." src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88049> s/Long/long/? src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88051> In general, avoid javadoc that re-states the signature in english. "@return Job update summaries matching the query." src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java <https://reviews.apache.org/r/24521/#comment88052> @return All stored job update details. src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java <https://reviews.apache.org/r/24521/#comment88063> Please add a disclaimer here: <p> NOTE: We don't want store serialized thrift objects long-term, but instead plan to reference a canonical table of task configurations. This class will go away with AURORA-647. src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java <https://reviews.apache.org/r/24521/#comment88066> Unless there's a need right now, don't even provide an abstract base - this is technical debt we shouldn't encourage further. src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java <https://reviews.apache.org/r/24521/#comment88064> Thrift binary-encoded byte array. src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java <https://reviews.apache.org/r/24521/#comment88065> s/ / / src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml <https://reviews.apache.org/r/24521/#comment88070> A comment explaining the "AS" table naming convention would be really nice. Clearly there's a reason, so it's nice to give a brief explanation. src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml <https://reviews.apache.org/r/24521/#comment88071> Why IN rather than =? If this is legit, please drop in a comment. src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql <https://reviews.apache.org/r/24521/#comment88072> s/BIGINT // src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql <https://reviews.apache.org/r/24521/#comment88073> s/BIGINT // src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/24521/#comment88075> Instance IDs to act on. All instances will be affected if this is not set. src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/24521/#comment88076> s/User/User who/ src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java <https://reviews.apache.org/r/24521/#comment88077> Query for an empty set of statuses would be a nice corner case to try as well. src/test/java/org/apache/aurora/scheduler/storage/db/DbUtil.java <https://reviews.apache.org/r/24521/#comment88078> AFAICT this can go away now as well. - Bill Farner On Aug. 12, 2014, 6:58 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24521/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2014, 6:58 p.m.) > > > Review request for Aurora, David McLaughlin and Bill Farner. > > > Bugs: AURORA-612 > https://issues.apache.org/jira/browse/AURORA-612 > > > Repository: aurora > > > Description > ------- > > Adding support for storing task configs and updateOnlyTheseInstances option. > Implementing the rest of defined JobUpdateStore APIs. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java > a9a325b877898be8a265c45112757deba0c3583f > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java > 808595817c31f3ef3515b623bbeb575bbf1f73fe > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java > 87f428be892204b8149170d39d3ace2962abc4bd > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java > 2a75646867a5af246625f7cf3910fd9a1bbf4f03 > src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > 5b7a3df035e78832ba4e6b595202d06af5d664a5 > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java > c5468b16709e7bc4758a0597bbe14257b31686ab > > src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml > ce4912d70fe75ea97f3d1bfb7fc4d5ec437fc83b > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > 3c69bda9fcb473205a3e308b15ad18cd1ae3cba7 > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > f0c8336cde4d262bcaf99921c556875ba819b05c > src/main/thrift/org/apache/aurora/gen/api.thrift > 4ea9ec2b96fc12429f33336b53e677e12662ec9a > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java > c76ab5cbde907a352dce25da585951831733487d > > src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java > 4c8238955464960e14ab858dcff7a53a64fcd72a > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java > 7ed6691f497ebaaaeb43d522af1358cc0b9491af > src/test/java/org/apache/aurora/scheduler/storage/db/DbQuotaStoreTest.java > 1f0af8669b655f773faa13a69887c6b0eb57dc57 > > src/test/java/org/apache/aurora/scheduler/storage/db/DbSchedulerStoreTest.java > 64e2fee337be597f6af4ffaa356872d34d316530 > src/test/java/org/apache/aurora/scheduler/storage/db/DbUtil.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 548322be029c6663a75187d9f341e53c5b7ca416 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > cbb9c468bf64691e573762f3734528c7b2e16490 > src/test/resources/org/apache/aurora/gen/api.thrift.md5 > 6bc6ccf37a34bfe6baf260034584be758a4c83f5 > > Diff: https://reviews.apache.org/r/24521/diff/ > > > Testing > ------- > > gradle -Pq build > ./pants src/test/python/apache/aurora/client/api:scheduler_client > > > Thanks, > > Maxim Khutornenko > >
