aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 35793: DbTaskStore: delete unreferenced job keys.
Date Thu, 25 Jun 2015 18:29:21 GMT


> On June 25, 2015, 6:05 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 254
> > <https://reviews.apache.org/r/35793/diff/3/?file=991667#file991667line254>
> >
> >     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
> > <https://reviews.apache.org/r/35793/diff/3/?file=991672#file991672line60>
> >
> >     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: 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message