[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/18192 Thank you @zhengcanbin and @jerryshao. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 @HyukjinKwon OK --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/18192 Okay, I won't go merging it without an explicit +1. @zhengcanbin, would you mind closing this PR/JIRA, and watching PRs and suggesting this fix latter? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user jerryshao commented on the issue: https://github.com/apache/spark/pull/18192 I'm neutral to this changes, personally I don't think it is quite necessary to do such changes here. So my response is +0. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/18192 @jerryshao, are you still not fond of this change as is? I was thinking of suggesting closing this or just merging rather than leaving open. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 @zsxwing --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/18192 I guess it is discouraged to move codes alone as explained but wouldn't it be better to merge this rather than close if this looks better in any way and the change is safe? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 @jerryshao Should I close this issue ? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user jerryshao commented on the issue: https://github.com/apache/spark/pull/18192 The change should be safe, but usually we don't do such code structure refactoring alone without a strong reason, so I'm neutral of this change. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 @jerryshao It's a tiny change for more reasonable code structure. There exists three `ShuffleWriter ` implementations, we first use the helper method `SortShuffleWriter#shouldBypassMergeSort` to determine whether a shuffle should use `BypassMergeSort` pathï¼and then use another helper method `SortShuffleManager#canUseSerializedShuffle` for deciding `UnsafeShuffleWriter` path. From view of code structure consistencyï¼method `shouldBypassMergeSort` should not belong to `SortShuffleWriter`ï¼it should be included in `BypassMergeSortShuffleWriter` or `SortShuffleManager`, and better for the later one to put the two helper methods together. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user jerryshao commented on the issue: https://github.com/apache/spark/pull/18192 @zhengcanbin can you please clarify the benefit of your changes? I don't see a big difference here. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 Tks @HyukjinKwon --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/18192 I think he suggested to fix the title to be complete (the current title of this PR looks truncated with "...". ) and describe what this PR proposes in the PR description with the PR template - https://github.com/apache/spark/blob/master/.github/PULL_REQUEST_TEMPLATE that would be seen before a PR is open. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user zhengcanbin commented on the issue: https://github.com/apache/spark/pull/18192 @heary-cao Tks, and what title you suggest and which comment format is correct ? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user heary-cao commented on the issue: https://github.com/apache/spark/pull/18192 Suggest modifying the title and Comment format is incorrect. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #18192: [SPARK-20944][SHUFFLE] Move shouldBypassMergeSort from S...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/18192 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
