You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/08/07 02:01:45 UTC
Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49998
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Aug. 8, 2014, 12:16 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 12:16 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
(Updated Aug. 8, 2014, 12:16 a.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
Merging JobUpdate and JobUpdateDetails mappers and adding more unit tests.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49971
-----------------------------------------------------------
Ship it!
LGTM once tests cover more than just the happy cases.
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/24431/#comment87430>
nit: use the same order for fields, constructor args, and assignments
- Bill Farner
On Aug. 7, 2014, 11:05 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 11:05 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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 11:05 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
Dropping type from all IDENTITY columns in schema.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 10:27 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
Somehow (and I hate this uncertainty), the cascading deletes now work again without FK constraint error. Returning them back to the schema.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:40 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
Dropping cascade deletes.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:34 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-612
https://issues.apache.org/jira/browse/AURORA-612
Repository: aurora
Description
-------
Adding SQL support for saving updates and fetching update details.
The configuration and updateOnlyTheseInstances option support will come separately.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
Diff: https://reviews.apache.org/r/24431/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2014, 4:39 a.m., Bill Farner wrote:
> > I missed this in a previous round, but i noticed that JobUpdate and JobUpdateSummary both have an "updateId" field. Should the one in JobUpdate go away?
> >
> > struct JobUpdate {
> > 1: string updateId
> > 2: JobUpdateSummary summary
> > ...
> > }
> >
> > struct JobUpdateSummary {
> > 1: string updateId
> > ...
> > }
Dropped from here and JobConfiguration. Only JobUpdateSummary now has it.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49863
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49863
-----------------------------------------------------------
I missed this in a previous round, but i noticed that JobUpdate and JobUpdateSummary both have an "updateId" field. Should the one in JobUpdate go away?
struct JobUpdate {
1: string updateId
2: JobUpdateSummary summary
...
}
struct JobUpdateSummary {
1: string updateId
...
}
- Bill Farner
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, line 28
> > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line28>
> >
> > If we're going to generate our own IDs, we should really let those be the foreign key references. However, i think now is a good time to revisit using the DB's generated ID.
> >
> > As far as i can tell, the current structure of JobUpdate works with this, since a JobUpdate is self-identifying.
> >
> > For objects that are not self-identifying, it looks like this would work:
> > http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
> >
> > In this case, insert() accepts Map<String, Object>, where one key points to the POJO, the other points to the returned autogenerated key. You would need this snippet below the insert statement:
> >
> > <selectKey resultType="long" order="AFTER" keyProperty="returnedId">
> > SELECT LAST_INSERT_ID()
> > </selectKey>
> >
> > Not terribly elegant, but avoids broader exposure of this limitation.
> >
>
> Bill Farner wrote:
> I played around with this idea on top of your patch, and it does work as expected. The exception, of course, is MERGE. In that case, you need a small workaround:
>
> MERGE INTO table (
> id,
> ...
> } VALUES (
> <if test="id == 0">
> null
> </if>
> <if test="id != 0">
> #{id}
> </if>,
> )
>
> Maxim Khutornenko wrote:
> I have actually made it work for task configs without a selectKey. I am passing a Result object into the insert() instead of Map. Will use this approach in the next CR.
>
> However, I am still concerned about it for updates. Relying on auto-generated keys requires stable IDs to guarantee correct relationships between tables. Since we re-populate the entire DB on failover, there are no hard guarantees the order will be preserved on re-insertion. Any shift in IDs on re-insertion into job_updates table would abandon/misplace records in event tables.
>
> Also, using pre-generated update IDs make tests simpler (i.e. no need to modify saved objects before asserting them against fetched ones).
> Since we re-populate the entire DB on failover, there are no hard guarantees
> the order will be preserved on re-insertion
If i understand you correctly - you mean an insert with a non-null id being replaced with an auto-generated value when we insert on failover? Seems like that should be easy to test against, barring non-deterministic behavior.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line 97
> > <https://reviews.apache.org/r/24431/diff/1/?file=654341#file654341line97>
> >
> > i think you can s/BIGINT //, AFAICT h2 uses a long regardless. Not a bad idea to do this without.
>
> Maxim Khutornenko wrote:
> Dropped. Now, I wonder if "INT IDENTITY" is actually meaningless. We should probably drop it everywhere.
Err, yeah, i had a typo - s/without/throughout/ in my comment. Please do make this change everywhere.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 9:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 9:40 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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 5d4da0abfd73e5ecbfb99ad81f51fca151124559
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 5d3ec36d9907be543296847134c53e6ddc6edc70
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/main/thrift/org/apache/aurora/gen/api.thrift 35d14d48f0c1ea5a4b67380e4c12b99245640a5d
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java a9545e1656a3b1b0ae55155eec4f06878158d8da
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java 48600b59b918434ef963b758279c48584ccd34ef
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 f01e4b33ee94ee7e8a36aa71688b699a87bc0566
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, line 28
> > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line28>
> >
> > If we're going to generate our own IDs, we should really let those be the foreign key references. However, i think now is a good time to revisit using the DB's generated ID.
> >
> > As far as i can tell, the current structure of JobUpdate works with this, since a JobUpdate is self-identifying.
> >
> > For objects that are not self-identifying, it looks like this would work:
> > http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
> >
> > In this case, insert() accepts Map<String, Object>, where one key points to the POJO, the other points to the returned autogenerated key. You would need this snippet below the insert statement:
> >
> > <selectKey resultType="long" order="AFTER" keyProperty="returnedId">
> > SELECT LAST_INSERT_ID()
> > </selectKey>
> >
> > Not terribly elegant, but avoids broader exposure of this limitation.
> >
>
> Bill Farner wrote:
> I played around with this idea on top of your patch, and it does work as expected. The exception, of course, is MERGE. In that case, you need a small workaround:
>
> MERGE INTO table (
> id,
> ...
> } VALUES (
> <if test="id == 0">
> null
> </if>
> <if test="id != 0">
> #{id}
> </if>,
> )
I have actually made it work for task configs without a selectKey. I am passing a Result object into the insert() instead of Map. Will use this approach in the next CR.
However, I am still concerned about it for updates. Relying on auto-generated keys requires stable IDs to guarantee correct relationships between tables. Since we re-populate the entire DB on failover, there are no hard guarantees the order will be preserved on re-insertion. Any shift in IDs on re-insertion into job_updates table would abandon/misplace records in event tables.
Also, using pre-generated update IDs make tests simpler (i.e. no need to modify saved objects before asserting them against fetched ones).
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, line 28
> > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line28>
> >
> > If we're going to generate our own IDs, we should really let those be the foreign key references. However, i think now is a good time to revisit using the DB's generated ID.
> >
> > As far as i can tell, the current structure of JobUpdate works with this, since a JobUpdate is self-identifying.
> >
> > For objects that are not self-identifying, it looks like this would work:
> > http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
> >
> > In this case, insert() accepts Map<String, Object>, where one key points to the POJO, the other points to the returned autogenerated key. You would need this snippet below the insert statement:
> >
> > <selectKey resultType="long" order="AFTER" keyProperty="returnedId">
> > SELECT LAST_INSERT_ID()
> > </selectKey>
> >
> > Not terribly elegant, but avoids broader exposure of this limitation.
> >
>
> Bill Farner wrote:
> I played around with this idea on top of your patch, and it does work as expected. The exception, of course, is MERGE. In that case, you need a small workaround:
>
> MERGE INTO table (
> id,
> ...
> } VALUES (
> <if test="id == 0">
> null
> </if>
> <if test="id != 0">
> #{id}
> </if>,
> )
>
> Maxim Khutornenko wrote:
> I have actually made it work for task configs without a selectKey. I am passing a Result object into the insert() instead of Map. Will use this approach in the next CR.
>
> However, I am still concerned about it for updates. Relying on auto-generated keys requires stable IDs to guarantee correct relationships between tables. Since we re-populate the entire DB on failover, there are no hard guarantees the order will be preserved on re-insertion. Any shift in IDs on re-insertion into job_updates table would abandon/misplace records in event tables.
>
> Also, using pre-generated update IDs make tests simpler (i.e. no need to modify saved objects before asserting them against fetched ones).
>
> Bill Farner wrote:
> > Since we re-populate the entire DB on failover, there are no hard guarantees
> > the order will be preserved on re-insertion
>
> If i understand you correctly - you mean an insert with a non-null id being replaced with an auto-generated value when we insert on failover? Seems like that should be easy to test against, barring non-deterministic behavior.
Maxim and i discussed this some more offline, and ultimately we decided that counting on the ability to specify a value for an IDENTITY column is not a great idea, and even if it works with H2 it may not with other DB vendors in the future. We'll stick with the self-generated updateId string.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, line 15
> > <https://reviews.apache.org/r/24431/diff/1/?file=654329#file654329line15>
> >
> > s/org.apache.aurora.gen.//
Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, line 18
> > <https://reviews.apache.org/r/24431/diff/1/?file=654329#file654329line18>
> >
> > ditto
> >
> > s/if/if it/
Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, line 20
> > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line20>
> >
> > -2 indent
Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml, line 21
> > <https://reviews.apache.org/r/24431/diff/1/?file=654337#file654337line21>
> >
> > After this change, can you go back and update DbLockStore to not catch PersistenceException?
Good point. Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 63
> > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line63>
> >
> > notNullColumn is new for us, can you add a comment describing why it is used?
This is actually the only way to avoid creating an empty collection element out of a NULL row produced by LEFT JOIN when there are no elements on the right side. We are lucky this attribute exists at all: https://issues.apache.org/jira/browse/IBATISNET-206
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 34
> > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line34>
> >
> > If you set columnPrefix="j_", you might be able to let mybatis' POJO mapping take care of the individual fields. I _think_ you'll still need to specify <id> though. If this works here, the approach might apply to other resultMaps in here as well.
Good suggestion. Refactored a bit to make this happen.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 65
> > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line65>
> >
> > columnPrefix on the <collection> should make fields auto-amp, with the exception of those that need special handling (id, typeHandler).
Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 68
> > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line68>
> >
> > Ditto re: columnPrefix
Done.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml, line 59
> > <https://reviews.apache.org/r/24431/diff/1/?file=654340#file654340line59>
> >
> > Should we start leaning on the database and let 'ON DELETE CASCADE' handle child tables appropriately? I realize now i did not do this in AttributeMapper.xml, but i think it's worth considering.
I tried and failed with that. Here is a related complaint: http://stackoverflow.com/questions/4151316/unable-to-cascade-delete-a-onetoone-member
Will stick with current solution until I figure out how to make it work.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line 97
> > <https://reviews.apache.org/r/24431/diff/1/?file=654341#file654341line97>
> >
> > i think you can s/BIGINT //, AFAICT h2 uses a long regardless. Not a bad idea to do this without.
Dropped. Now, I wonder if "INT IDENTITY" is actually meaningless. We should probably drop it everywhere.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, line 46
> > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line46>
> >
> > A test case for two updates with the full slew of child entries would be nice, to prove that they don't cross wires (lesson learned from my recent JOIN flub).
Added.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, line 85
> > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line85>
> >
> > please check that these objects are valid, not just present (equality rather than size).
> >
> > this implicitly tests the ordering, so you can skip the checks below.
Refactored.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, line 135
> > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line135>
> >
> > You could do this in an @After as a catch-all for every test case.
Added.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, line 136
> > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line136>
> >
> > In general, favor:
> >
> > assertEquals(Optional.<Foo>absent(), actual);
> >
> > over:
> >
> > assertFalse(actual.isPresent());
> >
> > The difference is subtle, but the former produces test output that shows the incorrect value, and will point more directly towards the problem.
Changed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, line 28
> > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line28>
> >
> > If we're going to generate our own IDs, we should really let those be the foreign key references. However, i think now is a good time to revisit using the DB's generated ID.
> >
> > As far as i can tell, the current structure of JobUpdate works with this, since a JobUpdate is self-identifying.
> >
> > For objects that are not self-identifying, it looks like this would work:
> > http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
> >
> > In this case, insert() accepts Map<String, Object>, where one key points to the POJO, the other points to the returned autogenerated key. You would need this snippet below the insert statement:
> >
> > <selectKey resultType="long" order="AFTER" keyProperty="returnedId">
> > SELECT LAST_INSERT_ID()
> > </selectKey>
> >
> > Not terribly elegant, but avoids broader exposure of this limitation.
> >
I played around with this idea on top of your patch, and it does work as expected. The exception, of course, is MERGE. In that case, you need a small workaround:
MERGE INTO table (
id,
...
} VALUES (
<if test="id == 0">
null
</if>
<if test="id != 0">
#{id}
</if>,
)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and
fetchDetails.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24431/#review49860
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/24431/#comment87252>
s/org.apache.aurora.gen.//
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/24431/#comment87253>
ditto
s/if/if it/
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml
<https://reviews.apache.org/r/24431/#comment87254>
-2 indent
src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml
<https://reviews.apache.org/r/24431/#comment87255>
If we're going to generate our own IDs, we should really let those be the foreign key references. However, i think now is a good time to revisit using the DB's generated ID.
As far as i can tell, the current structure of JobUpdate works with this, since a JobUpdate is self-identifying.
For objects that are not self-identifying, it looks like this would work:
http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
In this case, insert() accepts Map<String, Object>, where one key points to the POJO, the other points to the returned autogenerated key. You would need this snippet below the insert statement:
<selectKey resultType="long" order="AFTER" keyProperty="returnedId">
SELECT LAST_INSERT_ID()
</selectKey>
Not terribly elegant, but avoids broader exposure of this limitation.
src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml
<https://reviews.apache.org/r/24431/#comment87251>
After this change, can you go back and update DbLockStore to not catch PersistenceException?
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87256>
If you set columnPrefix="j_", you might be able to let mybatis' POJO mapping take care of the individual fields. I _think_ you'll still need to specify <id> though. If this works here, the approach might apply to other resultMaps in here as well.
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87257>
notNullColumn is new for us, can you add a comment describing why it is used?
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87258>
columnPrefix on the <collection> should make fields auto-amp, with the exception of those that need special handling (id, typeHandler).
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87259>
Ditto re: columnPrefix
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml
<https://reviews.apache.org/r/24431/#comment87261>
Should we start leaning on the database and let 'ON DELETE CASCADE' handle child tables appropriately? I realize now i did not do this in AttributeMapper.xml, but i think it's worth considering.
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml
<https://reviews.apache.org/r/24431/#comment87260>
remove newline
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql
<https://reviews.apache.org/r/24431/#comment87262>
i think you can s/BIGINT //, AFAICT h2 uses a long regardless. Not a bad idea to do this without.
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87268>
A test case for two updates with the full slew of child entries would be nice, to prove that they don't cross wires (lesson learned from my recent JOIN flub).
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87264>
please check that these objects are valid, not just present (equality rather than size).
this implicitly tests the ordering, so you can skip the checks below.
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87267>
You could do this in an @After as a catch-all for every test case.
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87266>
In general, favor:
assertEquals(Optional.<Foo>absent(), actual);
over:
assertFalse(actual.isPresent());
The difference is subtle, but the former produces test output that shows the incorrect value, and will point more directly towards the problem.
- Bill Farner
On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:01 a.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 SQL support for saving updates and fetching update details.
>
> The configuration and updateOnlyTheseInstances option support will come separately.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 869590ed3c43c64936af42e60d82f5a6938475d3
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 5d6521813e40eed5cf7e6e0285a7400624c4c5b2
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 8a573f7b04ecc37a19e3886f334f626af577839a
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 844714c08cdaf280c8862e933a95ab009bd9e464
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 618d5b72fd3078ef59571b5f2e33ecce3eb7386c
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java f014123348e2b229b2bb9be0dce424327ec0dfbe
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 3997f9833a7289a49e4f4972efc3f953674bd41b
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql eb2216ae22ae95370d0b5cd15dbb3d61fca79272
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24431/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>