You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by f7753 <gi...@git.apache.org> on 2016/07/16 09:23:18 UTC
[GitHub] spark pull request #14232: [SPARK-16587] The annotation for the abstract Cla...
GitHub user f7753 opened a pull request:
https://github.com/apache/spark/pull/14232
[SPARK-16587] The annotation for the abstract Class 'BlockTransferMessage' missing several kind of messages.
**# What changes were proposed in this pull request?**
Currently, the Messages used by shuffle: OpenBlock, UploadBlock, RegisterExecutor has a simple introduction for the usage, while the other three (StreamHandle, RegisterDriver, HearBeat) haven' t.
This PR added them.
**# How was this patch tested?**
Please let some commuters to review the added annotation to judge them.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/f7753/spark SPARK-16587
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14232.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #14232
----
commit c2e255f6d1acfb683223a6dadeee0fdeaef0840e
Author: MaBiao <ma...@gmail.com>
Date: 2016-07-16T09:15:10Z
[SPARK-16587] Add the missing annotation
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14232: [SPARK-16587] [CORE] The annotation for the abstr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14232#discussion_r71063864
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java ---
@@ -35,6 +35,9 @@
* shuffle service. It returns a StreamHandle.
* - UploadBlock is only handled by the NettyBlockTransferService.
* - RegisterExecutor is only handled by the external shuffle service.
+ * - StreamHandle is only handled by the OneForOneBlockFetcher.
+ * - RegisterDriver is only handled by the MesosExternalShuffleService.
+ * - HearBeat is only handled by the MesosExternalShuffleService.
--- End diff --
HeartBeat. (This could also be an HTML list.) I am not sure these docs match the others. They're describing what's handled by the Netty service for external shuffle block handler, not just a class where they're used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14232: [SPARK-16587] [CORE] The annotation for the abstract Cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14232
I'm not clear if this matches the intent of the other docs. I'd close this unless we can get a clearer picture of the intent of these messages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14232: [SPARK-16587] [CORE] The annotation for the abstract Cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14232
Hm, not sure what to do here. Unless we can get an informed double-check on the correctness of this, I think I'd just not do it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14232: [SPARK-16587] [CORE] The annotation for the abstr...
Posted by f7753 <gi...@git.apache.org>.
Github user f7753 commented on a diff in the pull request:
https://github.com/apache/spark/pull/14232#discussion_r71064072
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java ---
@@ -35,6 +35,9 @@
* shuffle service. It returns a StreamHandle.
* - UploadBlock is only handled by the NettyBlockTransferService.
* - RegisterExecutor is only handled by the external shuffle service.
+ * - StreamHandle is only handled by the OneForOneBlockFetcher.
+ * - RegisterDriver is only handled by the MesosExternalShuffleService.
+ * - HearBeat is only handled by the MesosExternalShuffleService.
--- End diff --
Is it right to modify them like below ?
StreamHandle is only handled by the external shuffle service
RegisterDriver & OpenBlock is only handled by the external shuffle service while the cluster in the mesos deploy mode.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14232: [SPARK-16587] [CORE] The annotation for the abstract Cla...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14232
@f7753 I think just listing the message types here is not super useful. Yes, new messages were added and not mentioned in the documentation, but then adding just "foo is used by bar" doesn't really help in understanding the protocol at all.
Could you close this PR please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14232: [SPARK-16587] [CORE] The annotation for the abstr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14232#discussion_r71064096
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java ---
@@ -35,6 +35,9 @@
* shuffle service. It returns a StreamHandle.
* - UploadBlock is only handled by the NettyBlockTransferService.
* - RegisterExecutor is only handled by the external shuffle service.
+ * - StreamHandle is only handled by the OneForOneBlockFetcher.
+ * - RegisterDriver is only handled by the MesosExternalShuffleService.
+ * - HearBeat is only handled by the MesosExternalShuffleService.
--- End diff --
Best to ask the author where possible; use Blame to see who wrote these parts. @aarondav ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14232: [SPARK-16587] [CORE] The annotation for the abstract Cla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14232
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14232: [SPARK-16587] [CORE] The annotation for the abstr...
Posted by f7753 <gi...@git.apache.org>.
Github user f7753 closed the pull request at:
https://github.com/apache/spark/pull/14232
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14232: [SPARK-16587] [CORE] The annotation for the abstr...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14232#discussion_r73622103
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/BlockTransferMessage.java ---
@@ -35,6 +35,9 @@
* shuffle service. It returns a StreamHandle.
* - UploadBlock is only handled by the NettyBlockTransferService.
* - RegisterExecutor is only handled by the external shuffle service.
+ * - StreamHandle is only handled by the OneForOneBlockFetcher.
--- End diff --
This is already covered in the documentation of `OpenBlock`, since `StreamHandle` is the reply to `OpenBlock`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org