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

Ship it!


Master (a350982) 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 Jan. 15, 2015, 2:27 a.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29915/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2015, 2:27 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1016
>     https://issues.apache.org/jira/browse/AURORA-1016
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> It is possible for CachedClusterState to add a null SlaveID to the multimap. 
> This null key can propagate to the Preemptor and cause the following NPE:
> ````
> W0114 20:57:59.565 THREAD149 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl.schedule: 
> Task scheduling unexpectedly
>  failed, will be retried
> java.lang.NullPointerException
>         at 
> com.google.common.base.Preconditions.checkNotNull(Preconditions.java:213)
>         at 
> com.google.common.collect.ImmutableCollection$ArrayBasedBuilder.add(ImmutableCollection.java:339)
>         at 
> com.google.common.collect.ImmutableSet$Builder.add(ImmutableSet.java:480)
>         at 
> com.google.common.collect.ImmutableSet$Builder.add(ImmutableSet.java:456)
>         at 
> com.google.common.collect.ImmutableCollection$Builder.addAll(ImmutableCollection.java:282)
>         at 
> com.google.common.collect.ImmutableCollection$ArrayBasedBuilder.addAll(ImmutableCollection.java:360)
>         at 
> com.google.common.collect.ImmutableSet$Builder.addAll(ImmutableSet.java:508)
>         at 
> org.apache.aurora.scheduler.async.preemptor.PreemptorImpl.findPreemptionSlotFor(PreemptorImpl.java:321)
>         at 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl.maybePreemptFor(TaskScheduler.java:249)
>         at 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl.scheduleTask(TaskScheduler.java:220)
>         at 
> com.twitter.common.inject.TimedInterceptor.invoke(TimedInterceptor.java:87)
>         at 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl$3.apply(TaskScheduler.java:192)
>         at 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl$3.apply(TaskScheduler.java:189)
>         at 
> org.apache.aurora.scheduler.storage.log.LogStorage$24.apply(LogStorage.java:608)
>         at 
> org.apache.aurora.scheduler.storage.log.LogStorage$24.apply(LogStorage.java:605)
>         at 
> org.apache.aurora.scheduler.storage.mem.MemStorage$3.apply(MemStorage.java:147)
>         at 
> org.apache.aurora.scheduler.storage.mem.MemStorage$3.apply(MemStorage.java:144)
>         at 
> org.apache.aurora.scheduler.storage.db.DbStorage.write(DbStorage.java:137)
>         at 
> org.mybatis.guice.transactional.TransactionalMethodInterceptor.invoke(TransactionalMethodInterceptor.java:101)
>         at 
> org.apache.aurora.scheduler.storage.mem.MemStorage.write(MemStorage.java:144)
>         at 
> com.twitter.common.inject.TimedInterceptor.invoke(TimedInterceptor.java:87)
>         at 
> org.apache.aurora.scheduler.storage.log.LogStorage.doInTransaction(LogStorage.java:605)
>         at 
> org.apache.aurora.scheduler.storage.log.LogStorage.write(LogStorage.java:638)
>         at 
> org.apache.aurora.scheduler.storage.CallOrderEnforcingStorage.write(CallOrderEnforcingStorage.java:122)
>         at 
> org.apache.aurora.scheduler.async.TaskScheduler$TaskSchedulerImpl.schedule(TaskScheduler.java:189)
>         at 
> com.twitter.common.inject.TimedInterceptor.invoke(TimedInterceptor.java:87)
>         at 
> org.apache.aurora.scheduler.async.TaskGroups$1.schedule(TaskGroups.java:136)
>         at 
> org.apache.aurora.scheduler.async.TaskGroups$2.run(TaskGroups.java:158)
>         at 
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:262)
>         at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:178)
>         at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:292)
>         at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
>         at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
>         at java.lang.Thread.run(Thread.java:745)
> ````
> 
> This patch prevents the addition of the null key into the multimap and 
> instead throws an `IllegalStateException`.
> 
> 
> Diffs
> -----
> 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29915/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Reply via email to