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 25753: Surface instance update status changes so they may be persisted.
Date Thu, 18 Sep 2014 17:22:30 GMT


> On Sept. 17, 2014, 9:04 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/SideEffect.java, line 45
> > <https://reviews.apache.org/r/25753/diff/2/?file=692885#file692885line45>
> >
> >     Missing xml docs for public methods.
> 
> Bill Farner wrote:
>     We usually don't doc accessor methods (and checkstyle is configured as such).  Do
you propose we alter that practice?

It's a bit strange for a public method to go completely silent. It takes a bit of searching
around (e.g. constructor docs) to figure out what the return type means. I personaly prefer
documenting all public methods just for consistency sake but I don't feel strong to suggest
rule modification.


- Maxim


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


On Sept. 18, 2014, 5:12 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25753/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2014, 5:12 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Rework outputs from OneWayJobUpdater so JobUpdateControllerImpl has visibility into internal
state machine transitions, which is the last bit necessary before we can start saving instance
update events.
> 
> Also contains a small bit of cleanup:
> - removed TODOs that have been addressed
> - made Optional<InstanceAction> a member of Result, rather than having a switch
to map between them.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java dc11abda72855f925a93913f57c5d0e15bc2e0ff

>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb

>   src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java 5a9af39ca619b90dfbac16b8fed55a3f7127cce3

>   src/main/java/org/apache/aurora/scheduler/updater/SideEffect.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java 453daeb0b1bb6c9d958e0265d0b7379e1b7a6558

>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39

>   src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java defdf6ea056b6524a8fc25f76be37f9ed0fad3e8

>   src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java 6169471388eabc83245fbe3b8abb2c3a38eb00e1

> 
> Diff: https://reviews.apache.org/r/25753/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> 
> 
> Thanks,
> 
> Bill Farner
> 
>


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