aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Shirchenko <cald...@gmail.com>
Subject Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
Date Fri, 24 Feb 2017 22:46:27 GMT


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > How does this work with preemption? Should we reject a configuration where tasks
can be preempted and reserve resources?

First pass is not to preempt at all for tasks needing a dynamic reservation. Will do a followup
patch for that. 

And yeah, I will add a config validation to reject a task if TierInfo is both reserved and
preemptible.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java, line 27
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646612#file1646612line27>
> >
> >     Can you revert the import glob here?

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 295
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646610#file1646610line295>
> >
> >     I think a comment here explaining that we do not want to veto offers for reservation
reasons because after we reach the timeout we can reserve it.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 158
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646610#file1646610line158>
> >
> >     It sems we are only doing read only operations here, so lets change the sigature
to only accept the `StoreProvider` interface for safety.
> >     
> >     Also, maybe we should rename this to "hasReachedReservationTimeout"?
> >     
> >     It's not clear what we are "waiting" for in the name.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java, line 141
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646608#file1646608line141>
> >
> >     Lets remove the commented out code here?

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java, line 47
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646607#file1646607line47>
> >
> >     I think replacing "reserved task" with "task with resoruced resources" is clearer.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java, line 34
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646606#file1646606line34>
> >
> >     Indentation is off here, I think we want one argument per line (as it was before).

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 420
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646605#file1646605line420>
> >
> >     I think we can make this .info and list the task id, offer and operations.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 418
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646605#file1646605line418>
> >
> >     Is there a reason why we need the reserve op to be at index 0?
> >     
> >     Could we instead just do `.add()`?
> >     
> >     `List<T>#add(int i, T)` is a smell to me.

I learned that reserve op had to come before launch operation, otherwise Mesos master would
not be happy and fail to launch the task.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 407
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646605#file1646605line407>
> >
> >     I think we can remove this line here.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 187
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646605#file1646605line187>
> >
> >     This seems like a formatting error. Please revert it.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 136
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646605#file1646605line136>
> >
> >     The comment here and the type don't match.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 365
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646604#file1646604line365>
> >
> >     You need to log the task id for this line to provide enough information for
someone debugging a live cluster.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 114
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646599#file1646599line114>
> >
> >     Whats the reason for the `components.size() == 4` here?

Leftover code. Vanished.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 122
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646604#file1646604line122>
> >
> >     You need to do `this.driverSettings = requireNonNull(driverSettings)` here to
guard against null pointers.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 358
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646604#file1646604line358>
> >
> >     A comment here explaining that we want to label all resources would make this
bit clearer.
> >     
> >     I don't think we do `clearXXX().addAllXXX()` anywhere else in the code.
> >     
> >     Also does protobuf not provide a `setXXX` for collections?

Yeah, this is the only place where we used to clear resources and readd them. Nothing changes.


There is a setResources method but I believe you end up doing the same thing by just looping
over the list and placing them into the correct index [1]

http://mesos.apache.org/api/latest/java/org/apache/mesos/Protos.ExecutorInfo.Builder.html#setResources(int,%20org.apache.mesos.Protos.Resource.Builder)


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, line 201
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line201>
> >
> >     Could you elaborate on what you mean by "cluster maintenance" here? Seems like
a copy and paste error from above.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, line 315
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line315>
> >
> >     This needs javadoc.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, line 332
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line332>
> >
> >     This equals is not the same as the other POJOs. Look at `UnusedResource` for
inspiration.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, line 270
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line270>
> >
> >     `Optional<List<T>>` is a smell to me.
> >     
> >     Is it possible to model this just as `List<T>` where the empty list is
equal to `Optional.absent()`?

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 74
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line74>
> >
> >     I noticed from the code below that the mutator methods only act on copies of
the teir info.
> >     
> >     Have you considered having the mutator methods only return copies?
> >     
> >     This means that the impementations of `unReserve` `reReserve` only return copies.
> >     
> >     This would be good for performance because we no longer need to return a copy
of tier info for every access, and we only do the copying when necessary.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 77
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line77>
> >
> >     Could we condense `unReserve()` and `reReserve` to just be `setReservation(bool
b)`?
> >     
> >     I think it is more inline with our style elsewhere for mutators.getTier

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 109
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line109>
> >
> >     If you accept my suggestion above about making copying implicit in the mutators,
this would need to become private.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/TierManager.java, line 105
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646597#file1646597line105>
> >
> >     In line with my comments above, it is important to note this returns a copy
for every access which is sort of undesriable considering how frequently we do this in the
scheduling loop.

Done.


> On Feb. 24, 2017, 9 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, line
85
> > <https://reviews.apache.org/r/56690/diff/3/?file=1646603#file1646603line85>
> >
> >     If you take my suggestion above to model `resourceList` as `List<T>` I
think this code would be cleaned up and be easier to read.

Done.


- Dmitriy


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


On Feb. 24, 2017, 12:28 a.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56690/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2017, 12:28 a.m.)
> 
> 
> Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji.
> 
> 
> Bugs: AURORA-1819
>     https://issues.apache.org/jira/browse/AURORA-1819
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This is an RFC (without tests) for dynamic reservations proposal. If there is consensus
on the approach, I will add tests. This patch was also tested locally and works as expected.
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f

>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80

>   src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb

>   src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa

>   src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca

>   src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d

>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5

>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49

>   src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122

>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9

>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 0d639f66db456858278b0485c91c40975c3b45ac

>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35

>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce

>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf

>   src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9

>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java da378e84ee65a658ff2382489d3ab6d5f6451b5f

>   src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8

>   src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840

>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f

>   src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1

>   src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8

>   src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61

>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777

> 
> Diff: https://reviews.apache.org/r/56690/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>


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