You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/09/17 22:46:37 UTC
Review Request 25753: Surface instance update status changes so they
may be persisted.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25753/
-----------------------------------------------------------
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
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 17, 2014, 9:04 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, line 202
> > <https://reviews.apache.org/r/25753/diff/2/?file=692884#file692884line202>
> >
> > Static import for SideEffect.InstanceUpdateStatus.WORKING?
Done for all status values used.
> 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.
We usually don't doc accessor methods (and checkstyle is configured as such). Do you propose we alter that practice?
> On Sept. 17, 2014, 9:04 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/SideEffect.java, line 94
> > <https://reviews.apache.org/r/25753/diff/2/?file=692885#file692885line94>
> >
> > Document?
Done.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25753/#review53743
-----------------------------------------------------------
On Sept. 17, 2014, 8:55 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25753/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:55 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
>
>
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25753/#review53743
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java
<https://reviews.apache.org/r/25753/#comment93557>
Static import for SideEffect.InstanceUpdateStatus.WORKING?
src/main/java/org/apache/aurora/scheduler/updater/SideEffect.java
<https://reviews.apache.org/r/25753/#comment93558>
Missing xml docs for public methods.
src/main/java/org/apache/aurora/scheduler/updater/SideEffect.java
<https://reviews.apache.org/r/25753/#comment93559>
Document?
- Maxim Khutornenko
On Sept. 17, 2014, 8:55 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25753/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:55 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
>
>
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25753/#review53753
-----------------------------------------------------------
Ship it!
Other than the docs, lgtm.
- David McLaughlin
On Sept. 17, 2014, 8:55 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25753/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:55 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
>
>
Re: Review Request 25753: Surface instance update status changes so
they may be persisted.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25753/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 8:55 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 (updated)
-----
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