Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-25 Thread Bill Farner 


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java,
> >  line 60
> > 
> >
> > I'd still recommend having a run counter to help us monitor collector 
> > availability/health.
> 
> Bill Farner wrote:
> Does the comment above change your opinion?  If not, perhaps a more 
> cross-cutting approach is stats on the states of all Services?
> 
> Maxim Khutornenko wrote:
> > If not, perhaps a more cross-cutting approach is stats on the states of 
> all Services?
> 
> That would be ideal. Happy to accept a TODO.

Filed https://issues.apache.org/jira/browse/AURORA-1373


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89393
---


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-25 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89404
---

Ship it!


Ship It!

- Maxim Khutornenko


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-25 Thread Maxim Khutornenko 


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 254
> > 
> >
> > This will use a regular single threaded executor, right? Any chance to 
> > provide a logging executor instance instead?
> 
> Bill Farner wrote:
> The executor is not determined by this, just the interval.  See 
> `executor()` [1] for the executor being used.  AbstractScheduledService shuts 
> down and logs when exceptions are thrown, and our /services endpoint provides 
> visibility when a service dies.  Our other AbstractScheduledServices use this 
> approach.
> 
> [1] 
> http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/AbstractScheduledService.html#executor()

Yeah, looks like they are doing it right:
```
private final Runnable task = new Runnable() {
  @Override public void run() {
lock.lock();
try {
  AbstractScheduledService.this.runOneIteration();
} catch (Throwable t) {
  try {
shutDown();
  } catch (Exception ignored) {
logger.log(Level.WARNING, 
"Error while attempting to shut down the service after 
failure.", ignored);
  }
  notifyFailed(t);
  throw Throwables.propagate(t);
} finally {
  lock.unlock();
}
  }
};
```


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java,
> >  line 60
> > 
> >
> > I'd still recommend having a run counter to help us monitor collector 
> > availability/health.
> 
> Bill Farner wrote:
> Does the comment above change your opinion?  If not, perhaps a more 
> cross-cutting approach is stats on the states of all Services?

> If not, perhaps a more cross-cutting approach is stats on the states of all 
> Services?

That would be ideal. Happy to accept a TODO.


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89393
---


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: http

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-25 Thread Bill Farner 


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 254
> > 
> >
> > This will use a regular single threaded executor, right? Any chance to 
> > provide a logging executor instance instead?

The executor is not determined by this, just the interval.  See `executor()` 
[1] for the executor being used.  AbstractScheduledService shuts down and logs 
when exceptions are thrown, and our /services endpoint provides visibility when 
a service dies.  Our other AbstractScheduledServices use this approach.

[1] 
http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/AbstractScheduledService.html#executor()


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java,
> >  line 60
> > 
> >
> > I'd still recommend having a run counter to help us monitor collector 
> > availability/health.

Does the comment above change your opinion?  If not, perhaps a more 
cross-cutting approach is stats on the states of all Services?


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89393
---


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Th

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-25 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89393
---


Looks great! Just a couple suggestions below.


src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java (line 254)


This will use a regular single threaded executor, right? Any chance to 
provide a logging executor instance instead?



src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
(line 60)


I'd still recommend having a run counter to help us monitor collector 
availability/health.


- Maxim Khutornenko


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-24 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89278
---

Ship it!


Master (d28bd4f) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On June 24, 2015, 10:46 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 24, 2015, 10:46 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> c31446c447c3385a4763b8a516827988e46cc480 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> 11c9c4ada400d51fc83e9e0de03108456be15fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> 335d7a95e797fe940e71b10da44cbd97edea69ac 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> f5829ac063272123995193caef5151e0d52d435b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-24 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/
---

(Updated June 24, 2015, 10:46 p.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Moved row cleanup to a periodic garbage collection routine.


Bugs: AURORA-1298
https://issues.apache.org/jira/browse/AURORA-1298


Repository: aurora


Description
---

DbTaskStore: delete unreferenced job keys.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
c31446c447c3385a4763b8a516827988e46cc480 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
11c9c4ada400d51fc83e9e0de03108456be15fdf 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
335d7a95e797fe940e71b10da44cbd97edea69ac 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
2dc3034a5e4389588b5f796ff8dfb06dbc9939b8 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
30e3469a9f69091b929a9243f84036fa2fdd0539 
  
src/main/java/org/apache/aurora/scheduler/storage/db/GarbageCollectedTableMapper.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
afdaa49d6cd5c135f6e4ddda2b6a45d189560e09 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
5e9ba823d08d2b46342e7722c9df5f2a349c97cf 
  src/main/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollector.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
3ada6286e6ef6e3302802b74eec6c46dd582dc10 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
7ee001f9c019a1e7b669ae5cec6088bf974a3746 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
f5829ac063272123995193caef5151e0d52d435b 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
8258fb102b7f5fca9635143ebaed542d43abeb9f 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
24cf52680b69e23f5ccbbcada0606975b0405d5b 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
63a784f843eb7edf9a13c623e5355169c7e8623b 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
  
src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/35793/diff/


Testing
---

Note that this removes a defensive branch wherein we checked for inbound config 
references before attempting to delete.  With this change, we proactively 
delete and count on foreign key constraints to prevent deletion of rows that 
are still referenced.  I propose we adopt this as our pattern for handling 
shared references, as it seems to be the most sane approach available.

A gotcha with this case is that i do not believe mybatis provides a 
vendor-neutral approach to identify a consistency violation, and the best 
signal is a generic `PersistenceException`.  This is unfortunate since we can't 
distinguish between a hopeless query with invalid syntax, a network 
disconnection, or the anticipated case of a consistency violation.


Thanks,

Bill Farner

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Bill Farner 


> On June 23, 2015, 8:12 p.m., Maxim Khutornenko wrote:
> > Discussed with Bill offline and it seems like having an offline periodic 
> > async GC processor would be a better long term solution. It would allow us 
> > to handle all cleanup in a single place but more importantly will remove 
> > the implicit perf tax applied on every GC-enabled transaction.

Thanks, this is a much more tidy approach.  Updated diff coming shortly.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89047
---


On June 23, 2015, 6:28 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 23, 2015, 6:28 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89047
---


Discussed with Bill offline and it seems like having an offline periodic async 
GC processor would be a better long term solution. It would allow us to handle 
all cleanup in a single place but more importantly will remove the implicit 
perf tax applied on every GC-enabled transaction.

- Maxim Khutornenko


On June 23, 2015, 6:28 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 23, 2015, 6:28 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Bill Farner 


> On June 23, 2015, 6:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java,
> >  line 133
> > 
> >
> > Any particular reason you've settled on an explicit removal call as 
> > opposed to a dedicated java trigger (1)? Triggers certainly have their 
> > pitfals but they may provide a cleaner solution and let us migrate easier 
> > to pure SQL triggers when H2 implements support for them (2)
> > 
> > (1) - http://www.h2database.com/html/features.html#triggers
> > (2) - http://www.h2database.com/html/roadmap.html

Biggest issue is coupling to h2.  H2 only supports triggers written in java, 
implementing their API.  This would present a pretty big hurdle for using a 
different vendor.


> On June 23, 2015, 6:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java,
> >  line 136
> > 
> >
> > A stat counter recording a successful removal would be nice here and 
> > below. That would help us monitoring cleanup health.

What's the shelf life of that?  This seems like something we won't want to 
sprinkle in each of the many forthcoming places we need to do this, and doesn't 
seem like we'd actually be able to alert on it.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89022
---


On June 23, 2015, 6:28 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 23, 2015, 6:28 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89022
---



src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
(line 132)


Any particular reason you've settled on an explicit removal call as opposed 
to a dedicated java trigger (1)? Triggers certainly have their pitfals but they 
may provide a cleaner solution and let us migrate easier to pure SQL triggers 
when H2 implements support for them (2)

(1) - http://www.h2database.com/html/features.html#triggers
(2) - http://www.h2database.com/html/roadmap.html



src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
(line 135)


A stat counter recording a successful removal would be nice here and below. 
That would help us monitoring cleanup health.


- Maxim Khutornenko


On June 23, 2015, 6:28 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 23, 2015, 6:28 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/#review89023
---

Ship it!


Master (68c4620) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On June 23, 2015, 6:28 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35793/
> ---
> 
> (Updated June 23, 2015, 6:28 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1298
> https://issues.apache.org/jira/browse/AURORA-1298
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> DbTaskStore: delete unreferenced job keys.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 30e3469a9f69091b929a9243f84036fa2fdd0539 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 3ada6286e6ef6e3302802b74eec6c46dd582dc10 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 7ee001f9c019a1e7b669ae5cec6088bf974a3746 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  8258fb102b7f5fca9635143ebaed542d43abeb9f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 24cf52680b69e23f5ccbbcada0606975b0405d5b 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 63a784f843eb7edf9a13c623e5355169c7e8623b 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> dda988d03634f8de582cf2b8ccdeb433c3e3de0c 
> 
> Diff: https://reviews.apache.org/r/35793/diff/
> 
> 
> Testing
> ---
> 
> Note that this removes a defensive branch wherein we checked for inbound 
> config references before attempting to delete.  With this change, we 
> proactively delete and count on foreign key constraints to prevent deletion 
> of rows that are still referenced.  I propose we adopt this as our pattern 
> for handling shared references, as it seems to be the most sane approach 
> available.
> 
> A gotcha with this case is that i do not believe mybatis provides a 
> vendor-neutral approach to identify a consistency violation, and the best 
> signal is a generic `PersistenceException`.  This is unfortunate since we 
> can't distinguish between a hopeless query with invalid syntax, a network 
> disconnection, or the anticipated case of a consistency violation.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.

2015-06-23 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35793/
---

(Updated June 23, 2015, 6:28 p.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Remove stale TODO.


Bugs: AURORA-1298
https://issues.apache.org/jira/browse/AURORA-1298


Repository: aurora


Description
---

DbTaskStore: delete unreferenced job keys.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
30e3469a9f69091b929a9243f84036fa2fdd0539 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
3ada6286e6ef6e3302802b74eec6c46dd582dc10 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
7ee001f9c019a1e7b669ae5cec6088bf974a3746 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
8258fb102b7f5fca9635143ebaed542d43abeb9f 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
24cf52680b69e23f5ccbbcada0606975b0405d5b 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
63a784f843eb7edf9a13c623e5355169c7e8623b 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
dda988d03634f8de582cf2b8ccdeb433c3e3de0c 

Diff: https://reviews.apache.org/r/35793/diff/


Testing
---

Note that this removes a defensive branch wherein we checked for inbound config 
references before attempting to delete.  With this change, we proactively 
delete and count on foreign key constraints to prevent deletion of rows that 
are still referenced.  I propose we adopt this as our pattern for handling 
shared references, as it seems to be the most sane approach available.

A gotcha with this case is that i do not believe mybatis provides a 
vendor-neutral approach to identify a consistency violation, and the best 
signal is a generic `PersistenceException`.  This is unfortunate since we can't 
distinguish between a hopeless query with invalid syntax, a network 
disconnection, or the anticipated case of a consistency violation.


Thanks,

Bill Farner