[SPARK-31344][CORE] Polish implementation of barrier() and allGather() by Ngone51 · Pull Request #28117 · apache/spark

Ngone51 · 2020-04-04T03:58:26Z

What changes were proposed in this pull request?

Combine BarrierRequestToSync and AllGatherRequestToSync into RequestToSync, which is distinguished by RequestMethod type.
Remove unnecessary Json serialization/deserialization
Clean up some codes to make runBarrier() and BarrierCoordinator more general
Remove unused imports.

Why are the changes needed?

To make codes simpler for better maintain in the future.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

This is pure code refactor, so should be covered by existed tests.

Ngone51 · 2020-04-04T04:02:48Z

This PR tried to address comment #27395 (review) , but please note that we can not simply build barrier() on top of allGather(""), because we need a way do distinguish these two different calls to prevent following illegal case:

rdd.barrier().mapPartitions { iter =>
   val context = BarrierTaskContext.get()
   val partitionId = context.partitionId
   if (partitionId == 0) {
     context.barrier()
   } else {
     context.allGather(partitionId.toString)
   }
  iter
 }

ping @sarthfrey @jiangxb1987 @mengxr Please take a look, thanks!

SparkQA · 2020-04-04T04:03:21Z

Test build #120795 has finished for PR 28117 at commit 2f18910.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-04T06:00:42Z

Test build #120796 has finished for PR 28117 at commit f95965e.

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

SparkQA · 2020-04-04T06:58:54Z

Test build #120797 has finished for PR 28117 at commit 617b80a.

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

Ngone51 · 2020-04-04T07:29:20Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

 private[spark] object RequestMethod extends Enumeration {
  val BARRIER, ALL_GATHER = Value
 }
+


TODO: remove redundant empty line.

sarthfrey

Thanks for cleaning this up @Ngone51, writing back the messages individually rather than parsing JSON + keeping a single case class seems to have removed a lot of bloat :)

sarthfrey · 2020-04-04T16:28:21Z

core/src/main/scala/org/apache/spark/BarrierTaskContext.scala

-    ()
-  }
+  def barrier(): Unit = runBarrier("", RequestMethod.BARRIER)
+


nit: extra line

jiangxb1987 · 2020-04-07T05:59:12Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala


-    // An Array of allGather messages for barrier tasks that have made a blocking runBarrier() call
-    private val allGatherMessages: ArrayBuffer[String] = new Array[String](numTasks).to[ArrayBuffer]
+    // Messages from each barrier task that have made a blocking runBarrier() call. And it will be


nit: And it -> The messages

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

SparkQA · 2020-04-07T10:31:45Z

Test build #120905 has finished for PR 28117 at commit e8d774b.

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

Ngone51 · 2020-04-09T08:07:38Z

Ping @mengxr @jiangxb1987

jiangxb1987 · 2020-04-09T18:48:16Z

LGTM, I'll wait for another couple days before merge this PR.

jiangxb1987 · 2020-04-15T07:12:17Z

retest this please

SparkQA · 2020-04-15T10:17:27Z

Test build #121307 has finished for PR 28117 at commit e8d774b.

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

jiangxb1987 · 2020-04-15T18:19:36Z

retest this please

SparkQA · 2020-04-15T21:09:32Z

Test build #121332 has finished for PR 28117 at commit e8d774b.

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

jiangxb1987 · 2020-04-16T17:57:08Z

retest this please

SparkQA · 2020-04-16T20:54:44Z

Test build #121372 has finished for PR 28117 at commit e8d774b.

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

jiangxb1987 · 2020-04-16T21:40:29Z

retest this please

SparkQA · 2020-04-17T00:13:51Z

Test build #121382 has finished for PR 28117 at commit e8d774b.

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

jiangxb1987 · 2020-04-17T00:21:23Z

retest this please

SparkQA · 2020-04-17T04:02:58Z

Test build #121384 has finished for PR 28117 at commit e8d774b.

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

jiangxb1987 · 2020-04-17T04:23:52Z

Thanks, merged to master!

Ngone51 · 2020-04-17T07:07:14Z

thanks all!

### What changes were proposed in this pull request? 1. Combine `BarrierRequestToSync` and `AllGatherRequestToSync` into `RequestToSync`, which is distinguished by `RequestMethod` type. 2. Remove unnecessary Json serialization/deserialization 3. Clean up some codes to make runBarrier() and `BarrierCoordinator` more general 4. Remove unused imports. ### Why are the changes needed? To make codes simpler for better maintain in the future. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? This is pure code refactor, so should be covered by existed tests. Closes apache#28117 from Ngone51/refactor_barrier. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Xingbo Jiang <xingbo.jiang@databricks.com>

refactor barrier

2f18910

Ngone51 added 3 commits April 4, 2020 12:08

fix python style

3f21ec4

remove unused imports

f95965e

fix python

617b80a

Ngone51 commented Apr 4, 2020

View reviewed changes

sarthfrey approved these changes Apr 4, 2020

View reviewed changes

jiangxb1987 reviewed Apr 7, 2020

View reviewed changes

address comments

e8d774b

jiangxb1987 closed this in b2e9e17 Apr 17, 2020

Conversation

Ngone51 commented Apr 4, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Ngone51 commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 4, 2020

Uh oh!

SparkQA commented Apr 4, 2020

Uh oh!

SparkQA commented Apr 4, 2020

Uh oh!

Ngone51 Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

sarthfrey left a comment

Choose a reason for hiding this comment

Uh oh!

sarthfrey Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Apr 7, 2020

Uh oh!

Ngone51 commented Apr 9, 2020

Uh oh!

jiangxb1987 commented Apr 9, 2020

Uh oh!

jiangxb1987 commented Apr 15, 2020

Uh oh!

SparkQA commented Apr 15, 2020

Uh oh!

jiangxb1987 commented Apr 15, 2020

Uh oh!

SparkQA commented Apr 15, 2020

Uh oh!

jiangxb1987 commented Apr 16, 2020

Uh oh!

SparkQA commented Apr 16, 2020

Uh oh!

jiangxb1987 commented Apr 16, 2020

Uh oh!

SparkQA commented Apr 17, 2020

Uh oh!

jiangxb1987 commented Apr 17, 2020

Uh oh!

SparkQA commented Apr 17, 2020

Uh oh!

jiangxb1987 commented Apr 17, 2020

Uh oh!

Ngone51 commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ngone51 commented Apr 4, 2020 •

edited

Loading