Support for scale down #1394

avflor · 2016-09-15T23:07:17Z

This is related to #1362

billonahill · 2016-09-16T17:23:20Z

heron/spi/src/java/com/twitter/heron/spi/packing/PackingPlan.java

@@ -229,6 +229,11 @@ public boolean equals(Object o) {
    }

    @Override
+    public int compareTo(ContainerPlan c) {
+      return Integer.compare(this.getId(), c.getId());


If we choose to implement Comparable we must assure that if thisContainer.compareTo(thatContainer) == 0 then thisContainer.equals(thatContainer), which isn't currently the case. It implies implementing Comparable for all child objects and including them in the comparison.

See http://docs.oracle.com/javase/8/docs/api/java/lang/Comparable.html#compareTo-T-

billonahill · 2016-09-16T17:26:45Z

heron/spi/tests/java/com/twitter/heron/spi/packing/PackingPlanTest.java

+        new InstanceId("bolt", 3, 0)));
+    packing.put(3, Arrays.asList(new InstanceId("spout", 2, 1)));
+    packing.put(2, Arrays.asList(new InstanceId("spout", 4, 1)));
+    PackingPlan packingPlan = generatePacking(packing);


Cleaner to use PackingTestUtils.testContainerPlan(..) repeatedly to construct an Array<ContainerPlan>.

billonahill · 2016-09-16T17:27:23Z

heron/spi/tests/java/com/twitter/heron/spi/packing/PackingPlanTest.java

+    packing.put(2, Arrays.asList(new InstanceId("spout", 4, 1)));
+    PackingPlan packingPlan = generatePacking(packing);
+
+    Object[] currentContainers = packingPlan.getContainers().toArray();


Best to work with ContainerPlan[] here instead of Object[].

billonahill · 2016-09-16T17:33:19Z

heron/packing/tests/java/com/twitter/heron/packing/AssertPacking.java

+      }
+    }
+    Assert.assertEquals("Components " + component + " found in the container plans: ",
+        instancesFound, numInstances);


The output of the assertion classes on failure expect the param ordering to be [expectedValue, foundValue].

billonahill · 2016-09-16T17:38:14Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

+        return instanceIds.get(i);
+      }
+    }
+    return null;


Please use Optional instead of null. See #1311

billonahill · 2016-09-16T18:11:41Z

heron/packing/src/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPacking.java

+   * @param firstTaskIndex first taskId to use for the new instances
+   */
+  private void assignInstancesToContainers(
+      ArrayList<Container> containers, Map<Integer, List<InstanceId>> allocation,


I feel like we're stretching our use of Lists and Maps in the signature here, by passing a mutable list of containers and a map of containers to instance lists. Instead can we use an object that encapsulated all of that? Like a PackingPlan object or a variant of it? Some container object that is able to effectively mutate a plan? Maybe a Builder that takes a packing plan, allows update methods to be called to change internal state, and then produces a new plan with a build() method?

billonahill · 2016-09-16T18:14:22Z

heron/packing/src/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPacking.java

@@ -313,4 +457,22 @@ private int allocateNewContainer(ArrayList<Container> containers) {
    containers.add(new Container(maxContainerResources));
    return containers.size();
  }
+
+  private class IdPair {


Can we use Pair instead? https://github.com/twitter/heron/blob/master/heron/common/src/java/com/twitter/heron/common/basics/Pair.java#L24

billonahill · 2016-09-16T18:18:57Z

heron/packing/src/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPacking.java

+        return new IdPair(containerId, instanceId);
+      }
+    }
+    return null;


Please use Optional instead of null. Or make this method return void if the caller doesn't care either way. See #1311

billonahill · 2016-09-16T18:20:04Z

heron/packing/src/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPacking.java

+  /**
+   * Removes containers from tha allocation that do not contain any instances
+   */
+  private void cleanAllocation(Map<Integer, List<InstanceId>> allocation) {


removeEmpyContainers(..)?

billonahill · 2016-09-16T18:28:52Z

heron/packing/src/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPacking.java

+   * Removes containers from tha allocation that do not contain any instances
+   */
+  private void cleanAllocation(Map<Integer, List<InstanceId>> allocation) {
+    Iterator<Map.Entry<Integer, List<InstanceId>>> entries = allocation.entrySet().iterator();


Instead of iterating over entries it's more idiomatic (and reads easier IMO since it's always obvious what the key and value represents by their naming) to iterate over keys:

Iterator<Integer> containerIds = allocation.keySet().iterator(); while (containerIds.hasNext() { Integer containerId = containerIds.next(); if (allocation.get(containerId) == null) allocation.remove(containerId); ...

Or even better using for (Integer containerId : allocation.keySet()) if that doesn't cause issues during deletion.

avflor · 2016-09-17T01:44:21Z

@billonahill I addressed all the comments except for the packing builder object. I need to think more about it.

billonahill · 2016-09-19T16:44:15Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

@@ -59,14 +67,48 @@ private boolean hasSpace(long ram, double cpuCores, long disk) {
   *
   * @return true if the instance can be added to the container, false otherwise
   */
-  public boolean add(Resource resource) {
+  public boolean add(Resource resource, InstanceId instanceId) {


An instance id with resources is an InstancePlan. Could we instead take an InstancePlan here and keep a Set of those? That way we don't need to separately track the sum of all resources for each add/remove.

If we do that we also don't need to pass a different resource to the remove method than the one that we know is bound to the instance. Currently the two aren't bound to each other, but there seems to be an assumption that the one passed is the same as that of the instance found and removed.

billonahill · 2016-09-19T16:47:30Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

+  public Optional<InstanceId> removeAnyInstanceOfComponent(Resource resource, String component) {
+    Optional<InstanceId> instanceId = getAnyInstanceOfComponent(component);
+    if (instanceId.isPresent()) {
+      usedRam -= resource.getRam();


When accessing class member variables, best to use self.usedRam here and elsewhere.

Feel free to disregard this comment, as we don't follow this standard in many places.

billonahill · 2016-09-19T16:50:12Z

heron/spi/src/java/com/twitter/heron/spi/packing/InstanceId.java

+
+  @Override
+  public String toString() {
+    return componentName + " " + taskId + " " + componentIndex;


Instead of string concatenation, better to use String.format("%s %d %d", ...)

billonahill · 2016-09-19T16:56:29Z

...n/packing/tests/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPackingTest.java

+    Assert.assertEquals(packingPlan.getContainers().size(), 2);
+    Assert.assertEquals(totalInstances, packingPlan.getInstanceCount());
+    AssertPacking.assertNumInstances(packingPlan.getContainers(), BOLT_NAME, 3);
+    AssertPacking.assertNumInstances(packingPlan.getContainers(), SPOUT_NAME, 4);


The first 7 lines here are duplicated in 5 tests. Could you pull those out into an initScalingTests() method or maybe something like this, followed by assertions on the returned plan:

PackingPlan doScalingTest(Map<String, Integer> componentChanges) {}

billonahill · 2016-09-19T17:12:05Z

heron/packing/src/java/com/twitter/heron/packing/PackingUtils.java

+   * @param containers
+   * @return sorted array of container plans
+   */
+  public static PackingPlan.ContainerPlan[] sortOnContainerId(


Could you add back the PackingPlanTest.testContainerSorting from the earlier review as PackingUtilsTest.testContainerSorting?

avflor · 2016-09-20T03:43:10Z

@billonahill I addressed all the comments. We have some ideas about the packing builder. Will submit that later.

billonahill · 2016-09-20T19:04:19Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

+    if (instancePlan.isPresent()) {
+      PackingPlan.InstancePlan plan = instancePlan.get();
+      this.instances.remove(plan);
+      return Optional.of(plan);


Instead of creating a new Optional you could return the same one removed: instancePlan.

billonahill · 2016-09-20T19:07:30Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

-  private long usedRam;
-  private double usedCpuCores;
-  private long usedDisk;
+  private HashSet<PackingPlan.InstancePlan> instances;

  //Maximum resources that can be assigned to the container.
  private long maxRam;
  private double maxCpuCores;
  private long maxDisk;

  public Container(Resource resource) {


to make it clear what this resource is, can we call it something like capacity and clarify it's use in the javadoc for the costructor?

billonahill · 2016-09-20T19:08:02Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

-  private long usedRam;
-  private double usedCpuCores;
-  private long usedDisk;
+  private HashSet<PackingPlan.InstancePlan> instances;

  //Maximum resources that can be assigned to the container.
  private long maxRam;


Instead of storing each part of the resource passed, can we just store the resource itself?

billonahill · 2016-09-20T19:13:02Z

heron/packing/tests/java/com/twitter/heron/packing/PackingUtilsTest.java

+  @Test
+  public void testContainerSortOnId() {
+
+    PackingPlan packingPlan = getFirstFitDecreasingPackingPlan(topology);


This is an indirect way to test container sorting. Could we instead write this test without a dep on FirstFitDecreasingPacking and just pass a Set of containers to sortOnContainerId and assert the returned ordering? That way we can directly test the utility method without going through the FirstFitDecreasingPacking code, which should be tested in it's own test suite/class.

billonahill · 2016-09-20T19:13:38Z

...n/packing/tests/java/com/twitter/heron/packing/binpacking/FirstFitDecreasingPackingTest.java

@@ -81,6 +81,35 @@ private static PackingPlan getFirstFitDecreasingPackingPlanRepack(
    return packing.repack(currentPackingPlan, componentChanges);
  }

+  PackingPlan doScalingTest(TopologyAPI.Topology t,


TopologyAPI.Topology topology

@billonahill This gives me a checkstyle error that that the topology hides a field because the class contains a topology member. This is the reason I used t.

You could make the method static and pass totalInstances in as totalExpectedInstances

billonahill · 2016-09-20T21:33:35Z

heron/packing/src/java/com/twitter/heron/packing/Container.java

+   * Creates a container with a specific capacity
+   * @param maxResource the capacity of the container in terms of cpu, ram and disk
+   */
+  public Container(Resource maxResource) {


Instead of using both capacity and maxResource, how about just capacity?

billonahill · 2016-09-20T21:35:03Z

One last naming nit and then lgtm! Thanks!

avflor · 2016-09-20T22:45:49Z

@billonahill Thanks for the comments. I think we can merge now.

* AssignInsatnces takes into account previous state * Scale down support * Address review comments * Address comments in second review * Addressed review comments

avflor force-pushed the scaledown branch from dc46228 to 8b63069 Compare September 15, 2016 23:44

billonahill assigned avflor Sep 16, 2016

billonahill added this to the 0.14.4 milestone Sep 16, 2016

billonahill added the new feature label Sep 16, 2016

billonahill suggested changes Sep 16, 2016

View reviewed changes

avflor added 4 commits September 16, 2016 17:29

rebase to master

56ce024

AssignInsatnces takes into account previous state

be86625

Scale down support

2cce3dd

Address review comments

6a3d4c0

avflor force-pushed the scaledown branch from 8b63069 to 6a3d4c0 Compare September 17, 2016 00:39

billonahill suggested changes Sep 19, 2016

View reviewed changes

Address comments in second review

be61256

billonahill reviewed Sep 20, 2016

View reviewed changes

Addressed review comments

aec9b01

avflor force-pushed the scaledown branch from 5a4a87a to aec9b01 Compare September 20, 2016 21:43

avflor changed the title ~~DO NOT MERGE: Support for scale down~~ Support for scale down Sep 20, 2016

billonahill approved these changes Sep 20, 2016

View reviewed changes

billonahill merged commit 9917f6f into apache:master Sep 20, 2016

nicknezis pushed a commit that referenced this pull request Sep 14, 2020

Support for scale down (#1394)

76621ae

* AssignInsatnces takes into account previous state * Scale down support * Address review comments * Address comments in second review * Addressed review comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for scale down #1394

Support for scale down #1394

avflor commented Sep 15, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016 •

edited

Loading

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

billonahill Sep 16, 2016

avflor commented Sep 17, 2016

billonahill Sep 19, 2016 •

edited

Loading

billonahill Sep 19, 2016

billonahill Sep 19, 2016

billonahill Sep 19, 2016

billonahill Sep 19, 2016 •

edited

Loading

billonahill Sep 19, 2016

avflor commented Sep 20, 2016

billonahill Sep 20, 2016

billonahill Sep 20, 2016 •

edited

Loading

billonahill Sep 20, 2016

billonahill Sep 20, 2016

billonahill Sep 20, 2016

avflor Sep 20, 2016

billonahill Sep 20, 2016

billonahill Sep 20, 2016

billonahill commented Sep 20, 2016

avflor commented Sep 20, 2016

Support for scale down #1394

Support for scale down #1394

Conversation

avflor commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billonahill Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avflor commented Sep 17, 2016

billonahill Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billonahill Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avflor commented Sep 20, 2016

Choose a reason for hiding this comment

billonahill Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billonahill commented Sep 20, 2016

avflor commented Sep 20, 2016

billonahill Sep 16, 2016 •

edited

Loading

billonahill Sep 19, 2016 •

edited

Loading

billonahill Sep 19, 2016 •

edited

Loading

billonahill Sep 20, 2016 •

edited

Loading