Add a scoring-based approach to modifying packing plan #1572

billonahill · 2016-11-18T20:46:03Z

Instances are often added and removed to containers based on a ranking of all containers. This adds methods to the PackingPlanBuilder API to make it easy to do so without requiring the caller to rank, sort and iterate through containers. Instead, packing algorithms can now add/remove instances by passing a class that scores Components based on some heuristic. Multiple scorers can be passed for primary, secondary, tertiary, etc ranking.

This patch includes an common implementation that is used for round robin placement, but other custom implementations can be passed to score based on balance, resource utilization, etc.

billonahill · 2016-11-21T21:37:18Z

Any comments on this one? I think this change will make it much easier for us to compose rankings for how instance placement and removal should occur.

avflor · 2016-11-21T22:05:43Z

@billonahill Will take a look later today

wangli1426 · 2016-11-22T08:24:43Z

heron/packing/src/java/com/twitter/heron/packing/builder/ContainerIdScorer.java

+package com.twitter.heron.packing.builder;
+
+/**
+ * Sorts containers ascending by container id. Id firstId and maxId as passed, starte with


a typo: starte -> start

wangli1426 · 2016-11-22T08:37:30Z

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

@@ -284,11 +285,13 @@ private void removeInstancesFromContainers(PackingPlanBuilder packingPlanBuilder
    ArrayList<RamRequirement> ramRequirements =
        getSortedRAMInstances(componentsToScaleDown.keySet());

+    ContainerIdScorer scorer = new ContainerIdScorer();


Is a reverse scorer for instance removal better? IMO, inserting and removing instances among the containers in reverse direct will result in better resource utilization.

wangli1426 · 2016-11-22T08:53:37Z

@billonahill I have reviewed this PR. It is smart to use a scorer here. Great work! I have two comments:

Since instance insertion is done by searching the containers from the smallest container id to the largest one until a container with enough resource is found, you may want to remove the instances of the scaling-down component from the containers in a reverse order. In this way, the container with the largest id is more likely to be empty eventually, making it possible to release this empty container in order to improve the resource utilization.
If a container becomes empty after a component scaled down, will the container be released?

Thanks.

billonahill · 2016-11-22T16:20:12Z

@wangli1426 thanks for the comments.

Yes, we should improve the scale down logic. In this PR though I'd keep all functionality the same and just changing the implementation to introduce the ranked approach. In a follow up we'll change the scoring logic for scale down, most likely when addressing Scaling down should remove instances from the container with the most imbalance #1560.
Yes, in the PackingPlanBuilder.pack() method, empty containers get removed from the built PackingPlan. This happens in the buildContainerPlans() method, where you'll see that we continue in the loop if the number of container instances is zero.

avflor · 2016-11-22T19:01:57Z

heron/packing/src/java/com/twitter/heron/packing/builder/PackingPlanBuilder.java

+   * @return containerId of the container the instance was removed from
+   * @throws com.twitter.heron.spi.packing.PackingException if the instance could not be removed
+   */
+  public int removeInstance(List<Scorer<Container>> scorers, String componentName) {


In the RCRR implementation, during scale down we do not always start from the first container. For example if we need to scale down 2 components by 1 instance from containers [1...5] and the first component is removed from instance 2 then the removal for the next component will start from container 3. This will help with load balancing. This remove instance method does not capture that. However, it is fine if a good scoring method is used when we change the scale down approach as discussed in the other thread.

@billonahill I'm not sure if that is the same. In the previous example if we pass as input to the constructor container id 3 and containrId 5 then the search will stop at 5, right? Wehat we ideally want is to start from 3 and use this sequence: 4,5,1,2. Is that behavior captured?

billonahill · 2016-11-22T19:12:51Z

I believe we've captured that used case. See how we use the two different
constructors to the ContainerIDScorer.

On Tue, Nov 22, 2016 at 11:02 AM avflor notifications@github.com wrote:

@avflor commented on this pull request.

In
heron/packing/src/java/com/twitter/heron/packing/builder/PackingPlanBuilder.java
#1572 (review):

* @throws com.twitter.heron.spi.packing.PackingException if the instance could not be removed

*/

public int removeInstance(Scorer scorer, String componentName) {

List<Scorer> scorers = new LinkedList<>();

scorers.add(scorer);

return removeInstance(scorers, componentName);

}

@SuppressWarnings("JavadocMethod")

/**

* Remove an instance from the first container possible ranked by score. If a scoring tie exists,

* uses the next scorer in the scorers list to break the tie.

* @return containerId of the container the instance was removed from

* @throws com.twitter.heron.spi.packing.PackingException if the instance could not be removed

*/

public int removeInstance(List<Scorer> scorers, String componentName) {

In the RCRR implementation, during scale down we do not always start from
the first container. For example if we need to scale down 2 components by 1
instance from containers [1...5] and the first component is removed from
instance 2 then the removal for the next component will start from
container 3. This will help with load balancing. This remove instance
method does not capture that. However, it is fine if a good scoring method
is used when we change the scale down approach as discussed in the other
thread.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1572 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABczvblIJ0Vv-h2nf9dQG0EG4sexTMbks5rAzwmgaJpZM4K26I-
.

billonahill · 2016-11-22T19:50:02Z

Yes, that's why the constructor requires the maxId, so it can loop back to
the lowest and continue.

On Tue, Nov 22, 2016 at 11:18 AM avflor notifications@github.com wrote:

@avflor commented on this pull request.

In
heron/packing/src/java/com/twitter/heron/packing/builder/PackingPlanBuilder.java
#1572:

* @throws com.twitter.heron.spi.packing.PackingException if the instance could not be removed

*/

public int removeInstance(Scorer scorer, String componentName) {

List<Scorer> scorers = new LinkedList<>();

scorers.add(scorer);

return removeInstance(scorers, componentName);

}

@SuppressWarnings("JavadocMethod")

/**

* Remove an instance from the first container possible ranked by score. If a scoring tie exists,

* uses the next scorer in the scorers list to break the tie.

* @return containerId of the container the instance was removed from

* @throws com.twitter.heron.spi.packing.PackingException if the instance could not be removed

*/

public int removeInstance(List<Scorer> scorers, String componentName) {

@billonahill https://github.com/billonahill I'm not sure if that is the
same. In the previous example if we pass as input to the constructor
container id 3 and containrId 5 then the search will stop at 5, right?
Wehat we ideally want is to start from 3 and use this sequence: 4,5,1,2. Is
that behavior captured?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1572, or mute the thread
https://github.com/notifications/unsubscribe-auth/AABczkOZ2XD-GVC2Q1_HNvBgW8okdkgVks5rA0AJgaJpZM4K26I-
.

…e increasing.

billonahill · 2016-11-23T18:11:51Z

@avflor @wangli1426 any other comments?

avflor · 2016-11-24T00:41:43Z

@billionahill LGTM

wangli1426 · 2016-11-24T01:11:03Z

LGTM

* Add a scoring-based approach to modifying packing plan * Also use the scoring approach for adding in FFDP * Added more unit tests are removed dead code * Fixed typo and added check for estimated new container size > 0 before increasing.

Add a scoring-based approach to modifying packing plan

da06734

billonahill self-assigned this Nov 18, 2016

Also use the scoring approach for adding in FFDP

484b5a8

billonahill mentioned this pull request Nov 18, 2016

NPE while scaling up a topology #1566

Closed

Added more unit tests are removed dead code

c1c549f

wangli1426 suggested changes Nov 22, 2016

View reviewed changes

avflor reviewed Nov 22, 2016

View reviewed changes

Fixed typo and added check for estimated new container size > 0 befor…

3a0c90b

…e increasing.

billonahill merged commit 9c1894a into apache:master Nov 24, 2016

billonahill deleted the billg/container_scoring branch November 24, 2016 00:51

billonahill added this to the 0.14.6 milestone Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a scoring-based approach to modifying packing plan #1572

Add a scoring-based approach to modifying packing plan #1572

billonahill commented Nov 18, 2016 •

edited

billonahill commented Nov 21, 2016

avflor commented Nov 21, 2016

wangli1426 Nov 22, 2016

wangli1426 Nov 22, 2016

wangli1426 commented Nov 22, 2016 •

edited

billonahill commented Nov 22, 2016

avflor Nov 22, 2016

avflor Nov 22, 2016

billonahill commented Nov 22, 2016

@avflor commented on this pull request.

billonahill commented Nov 22, 2016

@avflor commented on this pull request.

billonahill commented Nov 23, 2016

avflor commented Nov 24, 2016

wangli1426 commented Nov 24, 2016

Add a scoring-based approach to modifying packing plan #1572

Add a scoring-based approach to modifying packing plan #1572

Conversation

billonahill commented Nov 18, 2016 • edited

billonahill commented Nov 21, 2016

avflor commented Nov 21, 2016

wangli1426 Nov 22, 2016

Choose a reason for hiding this comment

wangli1426 Nov 22, 2016

Choose a reason for hiding this comment

wangli1426 commented Nov 22, 2016 • edited

billonahill commented Nov 22, 2016

avflor Nov 22, 2016

Choose a reason for hiding this comment

avflor Nov 22, 2016

Choose a reason for hiding this comment

billonahill commented Nov 22, 2016

@avflor commented on this pull request.

billonahill commented Nov 22, 2016

@avflor commented on this pull request.

billonahill commented Nov 23, 2016

avflor commented Nov 24, 2016

wangli1426 commented Nov 24, 2016

billonahill commented Nov 18, 2016 •

edited

wangli1426 commented Nov 22, 2016 •

edited