Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Aug 26, 2014

No description provided.

@SparkQA
Copy link

SparkQA commented Aug 26, 2014

QA tests have started for PR 2138 at commit e5ad9d3.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Aug 26, 2014

QA tests have finished for PR 2138 at commit e5ad9d3.

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

@SparkQA
Copy link

SparkQA commented Aug 26, 2014

QA tests have started for PR 2138 at commit 6058a58.

  • This patch merges cleanly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The BlockManagerMaster actually exists on executors too. This is the consequence of bad naming, but this class is really the "agent with which the BlockManager (on both executors and the driver) communicates with the driver"

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, you don't need to stop this only if this is the driver

Copy link
Member Author

Choose a reason for hiding this comment

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

As I mentioned below, only one instance of BlockManagerMaster should communicate with driver.
So far, driver itself communicates with DriverActor via SparkContext.stop invoked in user program or ApplicationMaster (Cluster mode).

@SparkQA
Copy link

SparkQA commented Aug 26, 2014

QA tests have finished for PR 2138 at commit 6058a58.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Aug 26, 2014

@andrewor14 Yes, Executor has a instance of BlockManagerMaster but all the important things BlockManagerMaster#stop do is send message "StopBlockManagerMaster" to DriverActor.

If we allow Executors to send the message, most of sending should be fail because DriverActor accept at most one message "StopBlockManagerMaster".

@SparkQA
Copy link

SparkQA commented Aug 29, 2014

QA tests have started for PR 2138 at commit d5ab19a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Aug 29, 2014

QA tests have finished for PR 2138 at commit d5ab19a.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

@sarutak This assumes that the only logic in blockManagerMaster.stop() is sending the stop message to the driver. While this is true in the current code, we may add logic there in the future that also affects the executors. I think the correct approach here is to add the conditional inside BlockManagerMaster#stop itself rather than in SparkEnv.

@sarutak
Copy link
Member Author

sarutak commented Sep 1, 2014

@andrewor14 Yeah, exactly. I'll try to implement conditional logic in BlockManagerMaster#stop itself.

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have started for PR 2138 at commit 4da8535.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have finished for PR 2138 at commit 4da8535.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to past this to SparkEnv itself. Just pass isDriver to the BlockManagerMaster

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have started for PR 2138 at commit 889e2d1.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have finished for PR 2138 at commit 889e2d1.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(var driverActor: ActorRef, conf: SparkConf, isDriver: Boolean) extends Logging

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have started for PR 2138 at commit 039b747.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 1, 2014

QA tests have finished for PR 2138 at commit 039b747.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(var driverActor: ActorRef,

Copy link
Contributor

Choose a reason for hiding this comment

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

The format should be

class BlockManagerMaster(
    var driverActor...
    conf:...
    isDriver:...
  extends Logging {

@andrewor14
Copy link
Contributor

1 minor comment and this LGTM

@sarutak
Copy link
Member Author

sarutak commented Sep 2, 2014

Thanks @andrewor14 .
I've modified format.

@SparkQA
Copy link

SparkQA commented Sep 2, 2014

QA tests have started for PR 2138 at commit d3005fd.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 2, 2014

QA tests have finished for PR 2138 at commit d3005fd.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2138 at commit c0205b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have finished for PR 2138 at commit c0205b7.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(

@sarutak
Copy link
Member Author

sarutak commented Sep 3, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2138 at commit c0205b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have finished for PR 2138 at commit c0205b7.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(

@sarutak
Copy link
Member Author

sarutak commented Sep 3, 2014

The failure was due to PageRank.
Jenkins, retest this please.

@sarutak
Copy link
Member Author

sarutak commented Sep 3, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2138 at commit c0205b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have finished for PR 2138 at commit c0205b7.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(

@andrewor14
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2138 at commit c0205b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have finished for PR 2138 at commit c0205b7.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(

@sarutak
Copy link
Member Author

sarutak commented Sep 3, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2138 at commit c0205b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 4, 2014

QA tests have finished for PR 2138 at commit c0205b7.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerBlockManagerAdded(time: Long, blockManagerId: BlockManagerId, maxMem: Long)
    • case class SparkListenerBlockManagerRemoved(time: Long, blockManagerId: BlockManagerId)
    • case class SparkListenerApplicationStart(appName: String, appId: Option[String], time: Long,
    • class BlockManagerMaster(
    • protected trait YarnAllocateResponse

@andrewor14
Copy link
Contributor

Thanks I merged this into master

@asfgit asfgit closed this in 4bba10c Sep 4, 2014
xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
…onManager etc.

Author: Kousuke Saruta <sarutak@oss.nttdata.co.jp>

Closes apache#2138 from sarutak/SPARK-3233 and squashes the following commits:

c0205b7 [Kousuke Saruta] Merge branch 'SPARK-3233' of github.com:sarutak/spark into SPARK-3233
064679d [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-3233
d3005fd [Kousuke Saruta] Modified Class definition format of BlockManagerMaster
039b747 [Kousuke Saruta] Modified style
889e2d1 [Kousuke Saruta] Modified BlockManagerMaster to be able to be past isDriver flag
4da8535 [Kousuke Saruta] Modified BlockManagerMaster#stop to send StopBlockManagerMaster message when sender is Driver
6518c3a [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-3233
d5ab19a [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-3233
6bce25c [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into SPARK-3233
6058a58 [Kousuke Saruta] Modified Executor not to invoke SparkEnv#stop in local mode
e5ad9d3 [Kousuke Saruta] Modified Executor to stop SparnEnv at the end of itself
@sarutak sarutak deleted the SPARK-3233 branch April 11, 2015 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants