> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > Does it make sense for the reconciler to run in parallel with the GC 
> > executor mechanism? It seems fine to me, but I would like some re-assurance 
> > here.

GC executor is not adding anything when task reconciliation is ON. Is there a 
particular use case you have in mind?


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 186
> > <https://reviews.apache.org/r/34440/diff/1/?file=964437#file964437line186>
> >
> >     This seems a little hacky, is there a reason why we cannot have an 
> > --enable_reconciliation flag? If disabled we can bind TaskReconciler to an 
> > implementation that does nothing.

I am hesitant adding yet another flag to accomplish what can already be done. 
This feature is not expected to be optional once GC executor is gone, so I 
don't see much motivation for extra complexity. Happy to reconsider though if 
others feel the same.


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskReconciler.java, line 43
> > <https://reviews.apache.org/r/34440/diff/1/?file=964438#file964438line43>
> >
> >     Please rename TIME to something that is more representitive of what it 
> > holds.

This holds what it tells (Time enum value). Any suggestions?


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskReconciler.java, line 96
> > <https://reviews.apache.org/r/34440/diff/1/?file=964438#file964438line96>
> >
> >     Linking to http://mesos.apache.org/documentation/latest/reconciliation/ 
> > or briefly explaining what is expected here would be nice.

Added a link to the class header.


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskReconciler.java, line 
> > 106
> > <https://reviews.apache.org/r/34440/diff/1/?file=964438#file964438line106>
> >
> >     Unrelated to this diff, but can you please explain how the 
> > StatusUpdates triggered from `reconcileTasks` are handled?

Status updates are handled via the existing MesosSchedulerImpl.statusUpdate() 
handling, which effectively routes the call to StateManager to attempt a task 
transition (or do nothing if states match).


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskReconciler.java, line 
> > 129
> > <https://reviews.apache.org/r/34440/diff/1/?file=964438#file964438line129>
> >
> >     perhaps we should call `executor.shutdownNow` here?

We never explicitly shutdown the executor but rather let it die along with the 
VM. Besides, it's a shared executor, which isn't created here.


> On May 19, 2015, 11:48 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java,
> >  line 67
> > <https://reviews.apache.org/r/34440/diff/1/?file=964441#file964441line67>
> >
> >     How is this related to this diff?

This is related to a bug I found in FakeScheduledExecutor. It was not honoring 
the initial delay the way normal scheduled executor does.


- Maxim


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


On May 19, 2015, 10:50 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34440/
> -----------------------------------------------------------
> 
> (Updated May 19, 2015, 10:50 p.m.)
> 
> 
> Review request for Aurora, Ben Mahler, Kevin Sweeney, and Zameer Manji.
> 
> 
> Bugs: AURORA-1047
>     https://issues.apache.org/jira/browse/AURORA-1047
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding a new service to request explict/implicit task reconciliations on a 
> periodic basis. The reconciler is OFF by default until the GC executor code 
> is removed.
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 316ab1c06ce63c9a3f7232264d30a40f487fc45c 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> e9d47fda3355786a4e68eac5c28490c04bc68cb3 
>   src/main/java/org/apache/aurora/scheduler/async/TaskReconciler.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 975ea02b45488608286e743888de25862cc77add 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 35cada6160af01c13362fa7c14b9ff8da0300004 
>   
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java
>  02e87989a17d95d36e61ffcef2e86c91774972bc 
>   src/test/java/org/apache/aurora/scheduler/async/TaskReconcilerTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/testing/FakeScheduledExecutor.java 
> 2beea4fc1a24c0050898077ecdf6cac2b19fab31 
> 
> Diff: https://reviews.apache.org/r/34440/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to