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/08/25 20:31:17 UTC
Review Request 61918: Fix concurrency issues around banned offers in
HostOffers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
Repository: aurora
Description
-------
Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
Diff: https://reviews.apache.org/r/61918/diff/1/
Testing
-------
Unit tests pass.
Currently running end to end test and testing on a live cluster.
Thanks,
Jordan Ly
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review183873
-----------------------------------------------------------
Master (364eac5) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 25, 2017, 1:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 1:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/1/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184010
-----------------------------------------------------------
Master (364eac5) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 25, 2017, 8:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 8:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/2/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On Aug. 28, 2017, 4:08 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
> > Line 361 (original), 367 (patched)
> > <https://reviews.apache.org/r/61918/diff/1/?file=1804129#file1804129line367>
> >
> > How about this method?
>
> Jordan Ly wrote:
> I only copy on the getOffers call for performance. I believe that the callers of getOffers do not expect the iterator to change.
>
> For `getWeaklyConsistentOffers`, we expect the caller to know that offers may be removed while iterating -- see the comment in launchTask:
>
> ```
> // Guard against an offer being removed after we grabbed it from the iterator.
> // If that happens, the offer will not exist in hostOffers, and we can immediately
> // send it back to LOST for quick reschedule.
> // Removing while iterating counts on the use of a weakly-consistent iterator being used,
> // which is a feature of ConcurrentSkipListSet.
> ```
Can you add comments here to explain the goal and usecase for the 2 different implementations? (`getWeaklyConsistentOffers` and `getOffers`)
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184005
-----------------------------------------------------------
On Aug. 25, 2017, 1:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 1:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/2/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Jordan Ly <jo...@gmail.com>.
> On Aug. 28, 2017, 11:08 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
> > Line 361 (original), 367 (patched)
> > <https://reviews.apache.org/r/61918/diff/1/?file=1804129#file1804129line367>
> >
> > How about this method?
I only copy on the getOffers call for performance. I believe that the callers of getOffers do not expect the iterator to change.
For `getWeaklyConsistentOffers`, we expect the caller to know that offers may be removed while iterating -- see the comment in launchTask:
```
// Guard against an offer being removed after we grabbed it from the iterator.
// If that happens, the offer will not exist in hostOffers, and we can immediately
// send it back to LOST for quick reschedule.
// Removing while iterating counts on the use of a weakly-consistent iterator being used,
// which is a feature of ConcurrentSkipListSet.
```
- Jordan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184005
-----------------------------------------------------------
On Aug. 25, 2017, 8:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 8:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/2/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184005
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
Line 357 (original), 360 (patched)
<https://reviews.apache.org/r/61918/#comment260027>
Drop the `FluentIterable.from(` since it is againg getting wrapped into the same below.
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
Line 361 (original), 367 (patched)
<https://reviews.apache.org/r/61918/#comment260028>
How about this method?
- Santhosh Kumar Shanmugham
On Aug. 25, 2017, 1:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 1:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/1/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184140
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
Lines 356-358 (original), 366-371 (patched)
<https://reviews.apache.org/r/61918/#comment260200>
I think we can avoid extra copies by replacing this with
```
return FluentIterable.from(offers).filter(
e -> !globallyBannedOffers.contains(e.getOffer().getId())
).toSet();
```
`toSet()` will only copy `offers` that pass the filter, and `globallyBannedOffers` will be not modified, as it will be called in synchronized context.
- Dmitry Zhuk
On Aug. 30, 2017, 2:06 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 2:06 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/3/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184119
-----------------------------------------------------------
Master (364eac5) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 30, 2017, 2:06 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 2:06 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/3/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184201
-----------------------------------------------------------
Master (364eac5) is red with this patch.
./build-support/jenkins/build.sh
[21] ./~/react-router-dom/es/Route.js 128 bytes {0} [built]
[26] ./~/react-router-dom/es/index.js 925 bytes {0} [built]
[27] ./src/main/js/index.js 1.75 kB {0} [built]
[38] ./~/preact/dist/preact.esm.js 29.6 kB {0} [built]
[43] ./~/react-router-dom/es/HashRouter.js 2.38 kB {0} [built]
[44] ./~/react-router-dom/es/MemoryRouter.js 149 bytes {0} [built]
[46] ./~/react-router-dom/es/Prompt.js 131 bytes {0} [built]
[47] ./~/react-router-dom/es/Redirect.js 137 bytes {0} [built]
[48] ./~/react-router-dom/es/StaticRouter.js 149 bytes {0} [built]
[49] ./~/react-router-dom/es/Switch.js 131 bytes {0} [built]
[50] ./~/react-router-dom/es/matchPath.js 140 bytes {0} [built]
+ 43 hidden modules
:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh
:checkstyleMain[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java:28:8: Unused import - com.google.common.collect.ImmutableSet. [UnusedImports]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
BUILD FAILED
Total time: 2 mins 41.274 secs
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 30, 2017, 9:24 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 9:24 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/4/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184209
-----------------------------------------------------------
Master (364eac5) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 30, 2017, 9:24 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 9:24 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/5/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184212
-----------------------------------------------------------
Ship it!
Thanks for the fix. LGTM!
- Stephan Erb
On Aug. 30, 2017, 11:24 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 11:24 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/5/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184255
-----------------------------------------------------------
Ship it!
Ship It!
- Dmitry Zhuk
On Aug. 30, 2017, 9:24 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 9:24 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/5/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 9:24 p.m.)
Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
Changes
-------
Avoid extra copies by using `.toSet()`
Repository: aurora
Description
-------
Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
Diff: https://reviews.apache.org/r/61918/diff/4/
Changes: https://reviews.apache.org/r/61918/diff/3-4/
Testing
-------
Unit tests pass.
Currently running end to end test and testing on a live cluster.
Thanks,
Jordan Ly
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 2:06 a.m.)
Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
Changes
-------
Added documentation.
Repository: aurora
Description
-------
Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
Diff: https://reviews.apache.org/r/61918/diff/3/
Changes: https://reviews.apache.org/r/61918/diff/2-3/
Testing
-------
Unit tests pass.
Currently running end to end test and testing on a live cluster.
Thanks,
Jordan Ly
Re: Review Request 61918: Fix concurrency issues around banned offers
in HostOffers
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61918/#review184012
-----------------------------------------------------------
Ship it!
Ship It!
- Santhosh Kumar Shanmugham
On Aug. 25, 2017, 1:31 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61918/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 1:31 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Dmitry Zhuk, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, `getOffers` and `getWeaklyConsistentOffers` provide iterables that use lambdas (in filter) not backed by concurrent data structures (`globallyBannedOffers` and `staticallyBannedOffers`). This can lead to a race condition where we have unknown behavior if reading and modification happens at the same time.
>
> To fix this, in `getOffers` we will revert to the previous behavior (before patch https://reviews.apache.org/r/61804/) where we create a copy of the current offers as well as a copy of the banned offers for consistency.
>
> For `getWeaklyConsistentOffers`, we will use concurrent data structures for `globallyBannedOffers` and `staticallyBannedOffers` so we don't have a race that may produce unknown side effects.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 5697d2e2eb001042511924442ff2dbe358451642
>
>
> Diff: https://reviews.apache.org/r/61918/diff/2/
>
>
> Testing
> -------
>
> Unit tests pass.
>
> Currently running end to end test and testing on a live cluster.
>
>
> Thanks,
>
> Jordan Ly
>
>