> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SideEffect.java, line 42
> > <https://reviews.apache.org/r/16873/diff/1/?file=423020#file423020line42>
> >
> >     Missing comments for public methods in this class?

We typically don't bother including a javadoc for one-liner methods.  If you 
feel strongly about this, you might want to push for the checkstyle config to 
require it.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 
> > 371
> > <https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line371>
> >
> >     s/SideEffect.Action/Action

Thanks, fixed.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 
> > 384
> > <https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line384>
> >
> >     \n

Done.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 
> > 439
> > <https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line439>
> >
> >     Might be more consistent and easier to follow if case statements are 
> > ordered by ACTIONS_IN_ORDER.

Sounds good, done.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 
> > 223
> > <https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line223>
> >
> >     It's hard to follow the logic here. Are both of these conditions 
> > desired here?
> >     false == false
> >     true == true
> >     Mind splitting it into two statements and adding proper error messages 
> > for each?

It's more than two statements, but i believe the desired effect is achieved in 
the new diff.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 
> > 553
> > <https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line553>
> >
> >     Better name for a parameter here to avoid confusion with the overload?

Done.


> On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 
> > 566
> > <https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line566>
> >
> >     Since it's fully public, might make sense moving it into a separate 
> > file?

Done.


- Bill


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


On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16873/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2014, 11:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Sorry for the monster diff.  The intertwined nature of StateManagerImpl and 
> TaskStateMachine made it pretty challenging to do this partially.  I hope the 
> end result is much easier to comprehend.
> 
> The "big picture" for this change is that the closures inside 
> TaskStateMachine no longer drop items onto a work queue that feeds back into 
> StateManagerImpl.  Instead, it returns these actions in a TransitionResult.  
> I intend to improve this further in the future by exposing only a helper 
> function in TaskStateMachine, to guarantee the one-time-use semantic.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/state/SideEffect.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 
> 2bdd4591f2182fe6c44d46f778be562d30eb2392 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 2b8ca095d8f108d516a43af8de4ff451ed9a8924 
>   src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 
> 11d283d31883b865b224d5af169dd5c42875021d 
>   src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java 
> aff74d535eb1237beafbcdf936d5ccc7101377c9 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 79f56052a25ba756208e747dc5d198f30f0c4900 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 7fe297a37aee0cb1be495e6a568b66271ee7bc3d 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 53793000de08fe80c0334241d332e3a50fca222a 
>   src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java 
> f44ee589430c2d4c0e014a705fd24b1f2fe08f36 
> 
> Diff: https://reviews.apache.org/r/16873/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to