> On 2012-01-10 14:26:24, John Sirois wrote: > > src/java/src/org/apache/mesos/Scheduler.java, line 57 > > <https://reviews.apache.org/r/3442/diff/1/?file=67453#file67453line57> > > > > There appears to be no need fo indexed access - Collection if you want > > to provide size, but it seems Iterable is fine here. > > Matei Zaharia wrote: > Going over the offers in the same order multiple times is useful in > complex schedulers -- for example, we do it in Spark. I suggest leaving it > this way, unless you want everyone to copy their Collection to a list. I > can't anticipate a scenario when it would be more efficient to use an > unordered collection here, and in other languages, such as Python, the > collection will be ordered anyway. > > John Sirois wrote: > The comment wasn't derived from efficiency concerns - a List could still > be passed since its Iterable. I was suggesting this change as an api > maintainer - Iterable gives you more freedom going forward to change > implementation without breaking clients and your clients still get a sequence > of offers they can repeatedly iterate over. You might add extra javadoc that > says the iterators are guaranteed to be stable wrt each other, but this is > implicit for all Iterables I've encountered in java land. > > Matei Zaharia wrote: > I realize that the comment isn't from efficiency concerns, but I'm just > saying that I can't imagine a case where the implementation would change in a > way that prevents meeting the List interface. Just to be clear, the code I'm > talking about doesn't simply want to iterate over the list multiple times -- > it wants to know the size of the list and keep some data about each machine > offered, and it does use indexes into it. For example, here's a common thing > to do: you have a list of tasks you want to launch and a list of machines > offered, and you want to spread your tasks across the machines in a > round-robin manner as long as they have enough resources. You can do > something like this: > > memoryLeft = // int array initialized to the memory on each machine > cpusLeft = // int array initialized to the CPUs on each machine > while (tasksToLaunch > 0) { > for (i = 0 to offers.size) { > if (memoryLeft[i] >= taskMemory && cpusLeft[i] >= taskCpus) { > launch a task on machine i > decrease memoryLeft[i] and cpusLeft[i] > } > } > } > > Basically, by not providing a List, you're losing some usability without > gaining much maintainability. I agree you shouldn't be providing an array or > something, but giving a less convenient collection type for something that is > already an ordered collection in other languages seems overly picky.
SGTM - I still don't get the need for an index since the slot an offer is in in the List has no mesos meaning afaict, but I can buy size. And I agree with your point that List is likely not too restrictive - I just tend to be _very_ defensive with exposed api types. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3442/#review4294 ----------------------------------------------------------- On 2012-01-10 06:48:03, Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3442/ > ----------------------------------------------------------- > > (Updated 2012-01-10 06:48:03) > > > Review request for mesos, Andy Konwinski, Charles Reiss, Matei Zaharia, John > Sirois, and Vinod Kone. > > > Summary > ------- > > Provides initial documentation to the public interfaces (thanks to Vinod Kone > for pairing with me on this). > > > This addresses bug MESOS-45. > https://issues.apache.org/jira/browse/MESOS-45 > > > Diffs > ----- > > include/mesos/executor.hpp d883f1a > include/mesos/mesos.proto 77a9067 > include/mesos/scheduler.hpp 93dda98 > src/java/src/org/apache/mesos/Executor.java 5ad8cee > src/java/src/org/apache/mesos/ExecutorDriver.java be01d21 > src/java/src/org/apache/mesos/MesosExecutorDriver.java 399c8b8 > src/java/src/org/apache/mesos/MesosSchedulerDriver.java 04809a6 > src/java/src/org/apache/mesos/Scheduler.java 63a06fe > src/java/src/org/apache/mesos/SchedulerDriver.java 23a246c > src/python/src/mesos.py 5fc60e2 > > Diff: https://reviews.apache.org/r/3442/diff > > > Testing > ------- > > > Thanks, > > Benjamin > >
