[GitHub] spark issue #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user jaceklaskowski commented on the issue: https://github.com/apache/spark/pull/16475 Closed as per @rxin's request. --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user rxin commented on the issue: https://github.com/apache/spark/pull/16475 Can we please close this? --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user jaceklaskowski commented on the issue: https://github.com/apache/spark/pull/16475 Partially agree @srowen. The reason for the change was `blockId.isShuffle` condition that both methods use to do their shuffle-specific handling. The change might not be the most correct one but I'm sure something's wrong here, but can't nail it and hence the change to review this part of shuffle system. > "is a distant problem waiting to happen" I was concerned about that too, but that's what the tests are supposed to cover. If there are few missing, they should be added, shouldn't they? --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user srowen commented on the issue: https://github.com/apache/spark/pull/16475 I'm not sure about this. The `getBlockData` method handles both the shuffle/non-shuffle case separately too -- and will actually call back to `getLocalBytes`. It is probably OK now, and won't result in an infinite recursion, but is a distant problem waiting to happen. It looks like this method also purposely intends to handle the shuffle case manually, itself. I don't know that this is an unambiguously good refactoring so would not make 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/16475 Merged build finished. Test PASSed. --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/16475 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/70914/ Test PASSed. --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/16475 **[Test build #70914 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/70914/testReport)** for PR 16475 at commit [`0c181ef`](https://github.com/apache/spark/commit/0c181ef686243c8c4c8138160fd8bad6fbc37953). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user jaceklaskowski commented on the issue: https://github.com/apache/spark/pull/16475 Proposed the changes since it made easier to understand the role of `getBlockData` vs `getLocalBytes` and in the end `ShuffleBlockResolver`. I'm not saying it should be accepted, but I'd like to hear your take on why it should not. Thanks! --- 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 #16475: [MINOR][CORE] Remove code duplication (so the interface ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/16475 **[Test build #70914 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/70914/testReport)** for PR 16475 at commit [`0c181ef`](https://github.com/apache/spark/commit/0c181ef686243c8c4c8138160fd8bad6fbc37953). --- 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
