Re: Review Request 24359: Fix announcer and scheduler_client tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24359/ --- (Updated Aug. 6, 2014, 4:11 p.m.) Review request for Aurora and Bill Farner. Changes --- Posting master merge before submitting. Bugs: AURORA-638 https://issues.apache.org/jira/browse/AURORA-638 Repository: aurora Description --- Fix tests. Diffs (updated) - src/test/python/apache/aurora/executor/common/test_announcer.py e5c4ce4cd0edf0666c5546b734b39121ce93a45c Diff: https://reviews.apache.org/r/24359/diff/ Testing --- Currently running ./pants src/test/python:all -v The announcer test does not fail on my laptop. But I predict what was happening is that since I was just doing 2 clock.tick(1.0)s with the ThreadedClock and the other end was doing a clock.sleep(2.0), there were no guarantees that code was getting executed unless I did a subsequent clock.tick(epsilon). I've instead just done clock.tick( 2) to guarantee that code executes. Thanks, Brian Wickman
Review Request 24393: Mark announcer expiration test as flaky.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24393/ --- Review request for Aurora and Bill Farner. Repository: aurora Description --- Mark announcer expiration test as flaky. Diffs - src/test/python/apache/aurora/executor/common/test_announcer.py df987b55750fa15674323cef6b2e8b7e5b6751ce Diff: https://reviews.apache.org/r/24393/diff/ Testing --- Test is now skipped: src/test/python/apache/aurora/executor/common/test_announcer.py ..s Thanks, Brian Wickman
Re: Review Request 24357: Refactor InstanceUpdater to remove the callback interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/#review49755 --- Heads up - i'll be sending a new diff shortly that collapses the two enums. This shouldn't change things too dramatically, but will result in less code. - Bill Farner On Aug. 6, 2014, 12:58 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/ --- (Updated Aug. 6, 2014, 12:58 a.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- When implementing the layer above InstanceUpdater, i found that the design required a circular parent/child dependency, among other awkwardness. The redesign should be much easier to work with, and IMO is easier to test. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 28635177ecd77626c5cb87b02dec225c26eea104 src/main/java/org/apache/aurora/scheduler/updater/TaskController.java b725a311647c1fa7d4344b359759d9f20e023533 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java a84536369be2ec7620bfa586fa85580773022d30 src/test/java/org/apache/aurora/scheduler/updater/TaskUtil.java PRE-CREATION Diff: https://reviews.apache.org/r/24357/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 24393: Mark announcer expiration test as flaky.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24393/#review49760 --- Ship it! Ship It! - Bill Farner On Aug. 6, 2014, 5:22 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24393/ --- (Updated Aug. 6, 2014, 5:22 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Mark announcer expiration test as flaky. Diffs - src/test/python/apache/aurora/executor/common/test_announcer.py df987b55750fa15674323cef6b2e8b7e5b6751ce Diff: https://reviews.apache.org/r/24393/diff/ Testing --- Test is now skipped: src/test/python/apache/aurora/executor/common/test_announcer.py ..s Thanks, Brian Wickman
Re: Review Request 24357: Refactor InstanceUpdater to remove the callback interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/ --- (Updated Aug. 6, 2014, 5:52 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Changes --- Now ready for a look. This should make the diff easier to deal with as well. Repository: aurora Description --- When implementing the layer above InstanceUpdater, i found that the design required a circular parent/child dependency, among other awkwardness. The redesign should be much easier to work with, and IMO is easier to test. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 28635177ecd77626c5cb87b02dec225c26eea104 src/main/java/org/apache/aurora/scheduler/updater/TaskController.java b725a311647c1fa7d4344b359759d9f20e023533 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java a84536369be2ec7620bfa586fa85580773022d30 src/test/java/org/apache/aurora/scheduler/updater/TaskUtil.java PRE-CREATION Diff: https://reviews.apache.org/r/24357/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 24357: Refactor InstanceUpdater to remove the callback interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/#review49782 --- Ship it! Ship It! - Maxim Khutornenko On Aug. 6, 2014, 5:52 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/ --- (Updated Aug. 6, 2014, 5:52 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- When implementing the layer above InstanceUpdater, i found that the design required a circular parent/child dependency, among other awkwardness. The redesign should be much easier to work with, and IMO is easier to test. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 28635177ecd77626c5cb87b02dec225c26eea104 src/main/java/org/apache/aurora/scheduler/updater/TaskController.java b725a311647c1fa7d4344b359759d9f20e023533 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java a84536369be2ec7620bfa586fa85580773022d30 src/test/java/org/apache/aurora/scheduler/updater/TaskUtil.java PRE-CREATION Diff: https://reviews.apache.org/r/24357/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 24357: Refactor InstanceUpdater to remove the callback interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24357/ --- (Updated Aug. 6, 2014, 7:28 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- People -= kevints Repository: aurora Description --- When implementing the layer above InstanceUpdater, i found that the design required a circular parent/child dependency, among other awkwardness. The redesign should be much easier to work with, and IMO is easier to test. Diffs - src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 28635177ecd77626c5cb87b02dec225c26eea104 src/main/java/org/apache/aurora/scheduler/updater/TaskController.java b725a311647c1fa7d4344b359759d9f20e023533 src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java a84536369be2ec7620bfa586fa85580773022d30 src/test/java/org/apache/aurora/scheduler/updater/TaskUtil.java PRE-CREATION Diff: https://reviews.apache.org/r/24357/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
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
Review Request 24432: Fix incorrect join type used in LockMapper.xml.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- Ship it! src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml https://reviews.apache.org/r/24432/#comment87223 How about INNER JOIN instead to be totally explicit? - Maxim Khutornenko On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? You and Kevin should has this one out. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. hash* - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. Bill Farner wrote: hash* Just don't see why we would use LEFT OUTER JOIN in some cases but JOIN in others. Why second guess what JOIN defaults to (INNER, NATURAL or CROSS)? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 6, 2014, 5:05 p.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. Bill Farner wrote: hash* Maxim Khutornenko wrote: Just don't see why we would use LEFT OUTER JOIN in some cases but JOIN in others. Why second guess what JOIN defaults to (INNER, NATURAL or CROSS)? Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 6, 2014, 5:03 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 6, 2014, 5:03 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 6, 2014, 5:05 p.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. Bill Farner wrote: hash* Maxim Khutornenko wrote: Just don't see why we would use LEFT OUTER JOIN in some cases but JOIN in others. Why second guess what JOIN defaults to (INNER, NATURAL or CROSS)? Kevin Sweeney wrote: Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere. In fact, for inner joins I prefer avoiding the JOIN keyword altogether like SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...) A CROSS JOIN is just: SELECT (...) FROM A a, B b WHERE (...) And a NATURAL JOIN (or any other type of strange join) is just: SELECT (...) FROM A a NATURAL JOIN B b WHERE (...) SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...) - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 6, 2014, 5:03 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 6, 2014, 5:03 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. Bill Farner wrote: hash* Maxim Khutornenko wrote: Just don't see why we would use LEFT OUTER JOIN in some cases but JOIN in others. Why second guess what JOIN defaults to (INNER, NATURAL or CROSS)? Kevin Sweeney wrote: Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere. Kevin Sweeney wrote: In fact, for inner joins I prefer avoiding the JOIN keyword altogether like SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...) A CROSS JOIN is just: SELECT (...) FROM A a, B b WHERE (...) And a NATURAL JOIN (or any other type of strange join) is just: SELECT (...) FROM A a NATURAL JOIN B b WHERE (...) SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...) | SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...) This is a pre SQL-92 syntax that is not compliant with the most recent SQL standards. E.g.: http://stackoverflow.com/questions/1599050/ansi-vs-non-ansi-sql-join-syntax In addition to the above, I don't see why I should be rewriting the query just to change from INNER JOIN to LEFT OUTER JOIN. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote: src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63 https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63 How about INNER JOIN instead to be totally explicit? Bill Farner wrote: You and Kevin should has this one out. Bill Farner wrote: hash* Maxim Khutornenko wrote: Just don't see why we would use LEFT OUTER JOIN in some cases but JOIN in others. Why second guess what JOIN defaults to (INNER, NATURAL or CROSS)? Kevin Sweeney wrote: Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere. Kevin Sweeney wrote: In fact, for inner joins I prefer avoiding the JOIN keyword altogether like SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...) A CROSS JOIN is just: SELECT (...) FROM A a, B b WHERE (...) And a NATURAL JOIN (or any other type of strange join) is just: SELECT (...) FROM A a NATURAL JOIN B b WHERE (...) SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...) Maxim Khutornenko wrote: | SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...) This is a pre SQL-92 syntax that is not compliant with the most recent SQL standards. E.g.: http://stackoverflow.com/questions/1599050/ansi-vs-non-ansi-sql-join-syntax In addition to the above, I don't see why I should be rewriting the query just to change from INNER JOIN to LEFT OUTER JOIN. Kevin wins by force of comment length. (read: i'm going to commit to fix this up, we can decide on the style async.) - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/#review49834 --- On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24432/ --- (Updated Aug. 7, 2014, 12:03 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-640 https://issues.apache.org/jira/browse/AURORA-640 Repository: aurora Description --- Fix incorrect join type used in LockMapper.xml. Diffs - src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681 src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec Diff: https://reviews.apache.org/r/24432/diff/ Testing --- Modified a test case to point out the issue. Thanks, Bill Farner
Re: 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/#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 MapString, 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.Fooabsent(), 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
Re: Review Request 24431: Implementing SQL mappers for saveUpdate and fetchDetails.
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 MapString, 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
