[SPARK-15354] [CORE] Topology aware block replication strategies #13932

shubhamchopra · 2016-06-27T23:23:03Z

What changes were proposed in this pull request?

Implementations of strategies for resilient block replication for different resource managers that replicate the 3-replica strategy used by HDFS, where the first replica is on an executor, the second replica within the same rack as the executor and a third replica on a different rack.
The implementation involves providing two pluggable classes, one running in the driver that provides topology information for every host at cluster start and the second prioritizing a list of peer BlockManagerIds.

The prioritization itself can be thought of an optimization problem to find a minimal set of peers that satisfy certain objectives and replicating to these peers first. The objectives can be used to express richer constraints over and above HDFS like 3-replica strategy.

How was this patch tested?

This patch was tested with unit tests for storage, along with new unit tests to verify prioritization behaviour.

shubhamchopra · 2016-07-22T20:31:43Z

Based on feedback from @rxin, added a Basic Strategy that replicates HDFS behavior as a simpler alternative to the constraint solver. I also ran some performance tests on the constraint solver and saw these numbers:

The times show average, min and max of 50 runs of the optimizer for 50, 100, ..., 100000 peers placed in appropriate number of racks. When blocks are being replicated, the majority of time is expected to be spent in the actual data movement across the network. These numbers show that the performance hit from the constraint solver can be expected to be minimal.

shubhamchopra · 2016-12-14T20:22:41Z

Rebased to master to resolve merge conflict

sameeragarwal · 2017-01-31T06:38:30Z

jenkins ok to test

SparkQA · 2017-01-31T06:43:40Z

Test build #72188 has started for PR 13932 at commit 93eb511.

shubhamchopra · 2017-02-02T22:46:47Z

No test errors. Looks like the test process was killed midway. Tests added as a part of this PR took less than 7s, so couldn't have caused the delay.

sameeragarwal · 2017-02-23T01:29:48Z

test this please

SparkQA · 2017-02-23T04:00:29Z

Test build #73311 has finished for PR 13932 at commit 93eb511.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-02-24T19:32:08Z

Test build #73435 has finished for PR 13932 at commit a35f673.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

shubhamchopra · 2017-02-27T21:00:41Z

Rebased to resolve merge conflicts.

shubhamchopra · 2017-02-27T21:08:29Z

test this please

SparkQA · 2017-02-27T21:10:34Z

Test build #73534 has started for PR 13932 at commit ec601bd.

cloud-fan · 2017-03-22T00:46:09Z

core/src/main/scala/org/apache/spark/storage/BlockReplicationPolicy.scala

shall we use LinkedHashSet so that we don't need this extra shuffle?

cloud-fan · 2017-03-22T00:50:10Z

core/src/main/scala/org/apache/spark/storage/BlockReplicationPolicy.scala

can we explain the replicating logic for any replication factor?

cloud-fan · 2017-03-22T02:25:58Z

retest this please

SparkQA · 2017-03-22T05:13:53Z

Test build #75020 has finished for PR 13932 at commit ec601bd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-23T18:50:50Z

Test build #75102 has finished for PR 13932 at commit 3d50cf3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-03-24T04:29:40Z

core/src/main/scala/org/apache/spark/storage/BlockReplicationPolicy.scala

shall we add a .filter(_.host != blockManagerId.host)?

Master ensures the list of peers sent to a block manager doesn't include the requesting block manager. Was that the intention here?

cloud-fan · 2017-03-24T04:35:28Z

core/src/test/scala/org/apache/spark/storage/BlockReplicationPolicySuite.scala

The previous indention was right.

cloud-fan · 2017-03-24T04:38:53Z

core/src/test/scala/org/apache/spark/storage/BlockReplicationPolicySuite.scala

shall we test more explicitly that the first candidate is within rack and the second candidate is outside rack?

The intended behavior is to ensure one is within rack and one outside, not necessarily the first or the second.

cloud-fan · 2017-03-24T04:39:23Z

LGTM except few minor comments

sameeragarwal · 2017-03-24T18:27:41Z

core/src/main/scala/org/apache/spark/storage/BlockReplicationPolicy.scala

~~Given that you're already shuffling the sample here anyways, just out of curiosity is there any advantage of using Robert Floyd's algorithm over (say) Fisher-Yates~~? Also, more generally, is space complexity really a concern here? Can't we just use r.shuffle(totalSize).take(sampleSize) for easy readability?

EDIT: Please ignore my first concern. I misread the code.

I completely agree with you here. Except I was told earlier that iterating through a list the size of the executors was a concern. So this was to address time complexity.

But isn't the time complexity same for both cases? It seems like they both only differ in terms of space complexity.

Note that, this logic is same as before, see https://github.com/apache/spark/pull/13932/files#diff-85cf7285f83b73c253480dc010b0013bL105

SparkQA · 2017-03-27T17:59:25Z

Test build #75264 has finished for PR 13932 at commit e70c2a1.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…tification.

…y to get peers making sure objectives previously satisfied are not violated.

…. 2. Fixing style issues

…zed.

…nagerReplicationSuite, to also run the same set of tests when using the basic strategy. Added a couple of specific test cases to verify prioritization.

…, along with test cases.

…cation policy.

…sed replication strategy and constraint solver associate with it.

shubhamchopra · 2017-03-28T14:49:20Z

Rebased to master

SparkQA · 2017-03-28T17:32:23Z

Test build #75315 has finished for PR 13932 at commit c465aaf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-03-29T12:34:34Z

retest this please

cloud-fan · 2017-03-29T12:35:09Z

I just merged a PR about block manager, retest this PR to make sure there is no conflict

SparkQA · 2017-03-29T15:19:24Z

Test build #75356 has finished for PR 13932 at commit c465aaf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-03-30T14:22:12Z

thanks, merging to master!

sameeragarwal · 2017-04-11T21:15:16Z

core/src/test/scala/org/apache/spark/storage/BlockReplicationPolicySuite.scala

+    }
  }

+  test("Peers in 2 racks") {


@shubhamchopra this test seems to be failing occasionally: https://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.storage.TopologyAwareBlockReplicationPolicyBehavior&test_name=Peers+in+2+racks. Can you please take a look? Thanks!

fixed by #17624

shubhamchopra mentioned this pull request Jul 27, 2016

[SPARK-15353] [CORE] Making peer selection for block replication pluggable #13152

Closed

shubhamchopra force-pushed the PrioritizerStrategy branch 2 times, most recently from f94084f to 2d1cecb Compare August 9, 2016 15:37

shubhamchopra force-pushed the PrioritizerStrategy branch from 2d1cecb to ccc6ae0 Compare November 1, 2016 19:16

shubhamchopra force-pushed the PrioritizerStrategy branch from 44817f4 to cff0966 Compare December 14, 2016 20:05

shubhamchopra force-pushed the PrioritizerStrategy branch from cff0966 to 93eb511 Compare January 23, 2017 20:22

shubhamchopra changed the title ~~[SPARK-15354] [CORE] [WIP] Topology aware block replication strategies~~ [SPARK-15354] [CORE] Topology aware block replication strategies Jan 23, 2017

shubhamchopra force-pushed the PrioritizerStrategy branch from a35f673 to ec601bd Compare February 27, 2017 20:59

cloud-fan reviewed Mar 22, 2017

View reviewed changes

cloud-fan reviewed Mar 24, 2017

View reviewed changes

core/src/test/scala/org/apache/spark/storage/BlockReplicationPolicySuite.scala Outdated

Copy link

Contributor

cloud-fan Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous indention was right.

cloud-fan reviewed Mar 24, 2017

View reviewed changes

sameeragarwal reviewed Mar 24, 2017

View reviewed changes

shubhamchopra added 20 commits March 28, 2017 10:41

Changes recommended by @HyukjinKwon to fix style issues.

e0729e6

Updating prioritizer api to use current blockmanager id for self iden…

0117dff

…tification.

Fixing style issues.

e32798c

Fixing style issues.

463f754

Adding a set-cover formulation for picking peers to replicate blocks.

6f0cffa

Adding newline to the end of file

daa7cb4

Modifying the optimizer to use a modified greedy optimizer. We now tr…

fa55d5c

…y to get peers making sure objectives previously satisfied are not violated.

Making sure we consider peers we have previously replicated to.

4cd86db

1. Fixing topology mapper class issue, so we instantiate it correctly…

69cfc45

…. 2. Fixing style issues

Adding an assertion in test case.

265a24e

Searching for the set of peer that meet most new objectives is optimi…

30ba3e9

…zed.

Adding a basic HDFS like block replication strategy. Re-using BlockMa…

0abdb3d

…nagerReplicationSuite, to also run the same set of tests when using the basic strategy. Added a couple of specific test cases to verify prioritization.

Running the suite for PrioritizerStrategy as well.

0a892f6

Renaming to follow naming convention.

485c9a1

Rebasing to master. Adding two strategies with numReplicas constraint…

88d60e2

…, along with test cases.

Fixing the test case to use the right conf parameter to set the repli…

f638f02

…cation policy.

Fixing style errors.

bdf69d6

Incorporating suggestions from @sameeragarwal. Removing objectives ba…

d17f6e9

…sed replication strategy and constraint solver associate with it.

Incorporating suggestions from @cloud-fan

794a720

Correcting indentation.

c465aaf

shubhamchopra force-pushed the PrioritizerStrategy branch from e70c2a1 to c465aaf Compare March 28, 2017 14:47

asfgit closed this in b454d44 Mar 30, 2017

sameeragarwal reviewed Apr 11, 2017

View reviewed changes

[SPARK-15354] [CORE] Topology aware block replication strategies #13932

[SPARK-15354] [CORE] Topology aware block replication strategies #13932

Uh oh!

Conversation

shubhamchopra commented Jun 27, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

shubhamchopra commented Jul 22, 2016

Uh oh!

shubhamchopra commented Dec 14, 2016

Uh oh!

sameeragarwal commented Jan 31, 2017

Uh oh!

SparkQA commented Jan 31, 2017

Uh oh!

shubhamchopra commented Feb 2, 2017

Uh oh!

sameeragarwal commented Feb 23, 2017

Uh oh!

SparkQA commented Feb 23, 2017

Uh oh!

SparkQA commented Feb 24, 2017

Uh oh!

shubhamchopra commented Feb 27, 2017

Uh oh!

shubhamchopra commented Feb 27, 2017

Uh oh!

SparkQA commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 22, 2017

Uh oh!

SparkQA commented Mar 22, 2017

Uh oh!

SparkQA commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhamchopra Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhamchopra Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 24, 2017

Uh oh!

sameeragarwal Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 27, 2017

Uh oh!

shubhamchopra commented Mar 28, 2017

Uh oh!

SparkQA commented Mar 28, 2017

Uh oh!

cloud-fan commented Mar 29, 2017

Uh oh!

cloud-fan commented Mar 29, 2017

Uh oh!

SparkQA commented Mar 29, 2017

Uh oh!

cloud-fan commented Mar 30, 2017

Uh oh!

shubhamchopra Mar 27, 2017 •

edited

Loading

shubhamchopra Mar 27, 2017 •

edited

Loading

sameeragarwal Mar 24, 2017 •

edited

Loading