[SPARK-18886][CORE][FOLLOWUP] fix legacyResetOnTaskLaunch version and method docs

[bmarcott]

What changes were proposed in this pull request?

fix version for config spark.locality.wait.legacyResetOnTaskLaunch
fix method return type doc

Why are the changes needed?

Incorrect docs/versioning for config

Does this PR introduce any user-facing change?

No

How was this patch tested?

Tests in PR

@viirya thanks, for catching these. good 👀
@cloud-fan
@tgravescs

[viirya]

ok to test

[SparkQA]

Test build #121655 has finished for PR 28307 at commit dabd83a.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[tgravescs]

test this please

[cloud-fan]

retest this please

[SparkQA]

Test build #121674 has finished for PR 28307 at commit dabd83a.

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

[cloud-fan]

retest this please

[SparkQA]

Test build #121679 has finished for PR 28307 at commit dabd83a.

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

[viirya]

retest this please

[SparkQA]

Test build #121689 has finished for PR 28307 at commit dabd83a.

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

[tgravescs]

I accidentally pushed the button to merge on here

[tgravescs]

Has anyone done that by accident and what is our protocol, should I revert it and use the script?
@cloud-fan @HyukjinKwon @dongjoon-hyun

[dongjoon-hyun]

Oh... I got it. I was wondering about the weird commit log without the committer. :)

[dongjoon-hyun]

I think it's okay for now since this is accident.

cc @srowen

[dongjoon-hyun]

BTW, how did you merge this into master and branch-3.0 together, @tgravescs ?

[tgravescs]

yeah the page was being really slow in loading and I clicked trying to put focus on the text box and it clicked the merge button. I should see if I can disable that.
I just clicked the squash and merge button, no confirmation or special options

[tgravescs]

I don't see it in branch-3, if it did we need to revert

[dongjoon-hyun]

Ya. My bad. I was confused. This is merged to master only.

[srowen]

If it was merged to master, and that was the intent, it's all fine. We don't need to revert. Just ensure the JIRA is resolved appropriately.

[tgravescs]

ok thanks. I was trying to kick the tests again to get them to pass this change here is purely documentation so wasn't causing it but like to get a clean run before merging.

[tgravescs]

test this please

[SparkQA]

Test build #121694 has finished for PR 28307 at commit dabd83a.

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

[HyukjinKwon]

TL;DR: I don't think this is a big problem.

I investigated on a topic related to this incident for a while before actually so I guess I have some more things to say here :-).

As far as I know Apache Parquet and some other Apache projects moved to Github Merge feature instead of the manual merging by the merger script. Practically it sounds better to me because we can now do many things such as closing PRs manually ever since we moved our repo.

I was investigating to propose to use Github Merge feature in Apache Spark too. However, Github team introduced a behaviour change on the commit credit, well, silently and suddenly - about the merger and author in the commit log.

The issue is open here isaacs/github#1303, and I gave them my feedback on this. It might be interesting to read the discussion there. As of now, the merger is not included, which I personally don't think it's ideal into Apache Spark at least. So, I stopped working on this for now.

I also discussed this with Github team offline, and seems they are working on a feature to run a script via a command such as @github merge as an alternative. Once they introduce such feature, we could discuss about migrating to that feature.

[dongjoon-hyun]

Thank you for the info, @HyukjinKwon !

[HyukjinKwon]

cc @greysteil FYI,

The context here is that one commit was accidentally merged by Github Merge feature instead of the merge script we use in this project - (can we disable the Github Merge feature BTW?).

I think this thread shows an example for what I wanted to elaborate before, how people think about the "committer" and the Github Merge feature, well, at least for this project specifically. Looks now at least it's not only me who thinks in this way :-).

As we discussed, "Github" is legitimately the "merger"; however, looks it might look a bit odd that the commit log does not involve the committer in practice.

[greysteil]

Sadly there's no way to disable the merge button. We actually have the same issue internally at GitHub, where we use a bot to merge and deploy PRs and the merge button is not used.

On the commit log, understood. I've already passed that feedback to the PR team here.

[tgravescs]

thanks for the info, is there anyway to get a confirmation prompt added to the merge button?

[greysteil]

Not at the moment. Hopefully a slip like the above is pretty rare, since it's already a two-click process?