Skip to content

Conversation

@lianhuiwang
Copy link
Contributor

Replace shuffleManagerClassName with shortShuffleMgrName is to reduce time of string's comparison. and put sort's comparison on the front. cc @JoshRosen @andrewor14

Copy link
Contributor

Choose a reason for hiding this comment

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

looks OK, but why not add a field in ShuffleManager itself that sets this? So you can just do shuffleManager.name or something here

@lianhuiwang
Copy link
Contributor Author

@andrewor14 Now I add shortShuffleMgrName to ShuffleManager for getting its short Name. How do you think? Thanks.

@SparkQA
Copy link

SparkQA commented Dec 4, 2015

Test build #47197 has finished for PR 10131 at commit c67cdca.

  • This patch passes all 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.

I would make these vals since we don't ever expect them to change within a shuffle manager

@andrewor14
Copy link
Contributor

Minor comment but this looks good otherwise.

@lianhuiwang
Copy link
Contributor Author

@andrewor14 thanks. I think now it's ok.

@SparkQA
Copy link

SparkQA commented Dec 15, 2015

Test build #47710 has finished for PR 10131 at commit 2259fba.

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

@SparkQA
Copy link

SparkQA commented Dec 15, 2015

Test build #47711 has finished for PR 10131 at commit 8bb4bce.

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

@SparkQA
Copy link

SparkQA commented Dec 15, 2015

Test build #47716 has finished for PR 10131 at commit 22e0e91.

  • This patch passes all 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.

just call this shortName

@andrewor14
Copy link
Contributor

LGTM, merging into master. I'll fix the comment on merge.

@asfgit asfgit closed this in 369127f Dec 16, 2015
@lianhuiwang
Copy link
Contributor Author

@andrewor14 OK, Thanks.

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