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

Reply via email to