You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Jordan Ly <jo...@gmail.com> on 2017/11/02 18:12:05 UTC
Review Request 63521: Fixed issue where saving attributes are not
being persisted to log
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63521/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Stephan Erb, and Bill Farner.
Repository: aurora
Description
-------
A bug was introduced when the old `MemAttributeStore` was revived. Previously, the `saveHostAttributes` method did not return anything. However, after migrating to the DB stores, the signature of the interface was changed to return a `boolean` if the save modified the previous attributes. The new changes accidentally inverted the order. The old `MemAttributeStoreTest` did not explicity test for this scenario so it went unnoticed.
The interface involved:
```
/**
* Save a host attribute in the attribute store.
*
* @param hostAttributes The attribute we are going to save.
* @return {@code true} if the operation changed the attributes stored for the given
* {@link IHostAttributes#getHost() host}, or {@code false} if the save was a no-op.
*/
boolean saveHostAttributes(IHostAttributes hostAttributes);
```
Diffs
-----
src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 483af194787e967a97c908a62889233336407aba
src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java 34db54be6eecbf0eaeab4fa2a19e6a66469cea88
Diff: https://reviews.apache.org/r/63521/diff/1/
Testing
-------
Fixed tests so they take into account the return value of the save.
Added a test explicity testing this behavior.
Thanks,
Jordan Ly
Re: Review Request 63521: Fixed issue where saving attributes are not
being persisted to log
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63521/#review189940
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Nov. 2, 2017, 11:12 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63521/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 11:12 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Stephan Erb, and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> A bug was introduced when the old `MemAttributeStore` was revived. Previously, the `saveHostAttributes` method did not return anything. However, after migrating to the DB stores, the signature of the interface was changed to return a `boolean` if the save modified the previous attributes. The new changes accidentally inverted the order. The old `MemAttributeStoreTest` did not explicity test for this scenario so it went unnoticed.
>
> The interface involved:
> ```
> /**
> * Save a host attribute in the attribute store.
> *
> * @param hostAttributes The attribute we are going to save.
> * @return {@code true} if the operation changed the attributes stored for the given
> * {@link IHostAttributes#getHost() host}, or {@code false} if the save was a no-op.
> */
> boolean saveHostAttributes(IHostAttributes hostAttributes);
> ```
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 483af194787e967a97c908a62889233336407aba
> src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java 34db54be6eecbf0eaeab4fa2a19e6a66469cea88
>
>
> Diff: https://reviews.apache.org/r/63521/diff/1/
>
>
> Testing
> -------
>
> Fixed tests so they take into account the return value of the save.
> Added a test explicity testing this behavior.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 63521: Fixed issue where saving attributes are not
being persisted to log
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63521/#review189948
-----------------------------------------------------------
Ship it!
Master (06d25fd) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 2, 2017, 6:12 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63521/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 6:12 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Stephan Erb, and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> A bug was introduced when the old `MemAttributeStore` was revived. Previously, the `saveHostAttributes` method did not return anything. However, after migrating to the DB stores, the signature of the interface was changed to return a `boolean` if the save modified the previous attributes. The new changes accidentally inverted the order. The `AbstractAttributeStoreTest` did not test for this scenario so it went unnoticed.
>
> The interface involved:
> ```
> /**
> * Save a host attribute in the attribute store.
> *
> * @param hostAttributes The attribute we are going to save.
> * @return {@code true} if the operation changed the attributes stored for the given
> * {@link IHostAttributes#getHost() host}, or {@code false} if the save was a no-op.
> */
> boolean saveHostAttributes(IHostAttributes hostAttributes);
> ```
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 483af194787e967a97c908a62889233336407aba
> src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java 34db54be6eecbf0eaeab4fa2a19e6a66469cea88
>
>
> Diff: https://reviews.apache.org/r/63521/diff/1/
>
>
> Testing
> -------
>
> Fixed tests so they take into account the return value of the save.
> Added a test explicity testing this behavior.
>
>
> Thanks,
>
> Jordan Ly
>
>