date:20140807

Re: Review Request 24431: Implementing SQL mappers for saveUpdate and fetchDetails.

2014-08-07 Thread Maxim Khutornenko



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

Re: Review Request 24431: Implementing SQL mappers for saveUpdate and fetchDetails.

2014-08-07 Thread Bill Farner

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.

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