Skip to content

Conversation

@vanzin
Copy link
Contributor

@vanzin vanzin commented Feb 19, 2019

Since the yarn module is actually private to Spark, this interface was never
actually "public". Since it has no use inside of Spark, let's avoid adding
a yarn-specific extension that isn't public, and point any potential users
are more general solutions (like using a SparkListener).

Since the yarn module is actually private to Spark, this interface was never
actually "public". Since it has no use inside of Spark, let's avoid adding
a yarn-specific extension that isn't public, and point any potential users
are more general solutions (like using a SparkListener).
@SparkQA
Copy link

SparkQA commented Feb 19, 2019

Test build #102515 has started for PR 23839 at commit 648083b.

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

is there deprecation note on the yarn doc page?

@dongjoon-hyun
Copy link
Member

Retest this please.

@vanzin
Copy link
Contributor Author

vanzin commented Feb 20, 2019

is there deprecation note on the yarn doc page?

I'm not sure it's even mentioned, but will check. If it's not, I'd rather just not call attention to it (it's never been mentioned in javadocs, for example).

@vanzin
Copy link
Contributor Author

vanzin commented Feb 20, 2019

No mention of the config key or the class name in the documentation, so I think we're good.

@SparkQA
Copy link

SparkQA commented Feb 20, 2019

Test build #102556 has finished for PR 23839 at commit 648083b.

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

@vanzin
Copy link
Contributor Author

vanzin commented Feb 20, 2019

Unrelated test failures... they're really weird, as if the YARN mini cluster stopped running containers for > 10 min. No idea what happened.

@vanzin
Copy link
Contributor Author

vanzin commented Feb 20, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Feb 21, 2019

Test build #102563 has finished for PR 23839 at commit 648083b.

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

@HyukjinKwon
Copy link
Member

cc @steveloughran

@vanzin
Copy link
Contributor Author

vanzin commented Feb 25, 2019

So lots of "approvals", but will anyone actually merge the change?

@srowen
Copy link
Member

srowen commented Feb 25, 2019

Go for it, I dont' see any objections and your judgment is obviously solid in this areas.

@vanzin
Copy link
Contributor Author

vanzin commented Feb 25, 2019

I know I can merge my own PRs, I'm just questioning this pattern that I see a lot where people approve PRs but nobody merges them.

@srowen
Copy link
Member

srowen commented Feb 25, 2019

For me personally it's that I know you can merge them and maybe want to let you pull the trigger as you know best that you don't have anything else you want to do with it, and also maybe which branches it should back port too. I'll merge this one.

@srowen
Copy link
Member

srowen commented Feb 25, 2019

(For non-committer PRs, I flag the email for followup in 1-3 days in Gmail, and yes I would then merge it, not just leave it approved indefinitely)

@srowen srowen closed this in 4808393 Feb 25, 2019
@vanzin vanzin deleted the SPARK-26788 branch March 1, 2019 17:54
@HyukjinKwon
Copy link
Member

Yup, I think in the same way of @srowen's.

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.

6 participants