Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jul 31, 2019

What changes were proposed in this pull request?

This PR proposes to improve the Github template for better and faster review iterations and better interactions between PR authors and reviewers.

As suggested in the the dev mailing list, this PR referred Kubernates' PR template.

Therefore, those fields are newly added:

### Why are the changes needed?
### Does this PR introduce any user-facing change?

and some comments were added.

Why are the changes needed?

Currently, many PR descriptions are poorly formatted, which causes some overheads between PR authors and reviewers.

There are multiple problems by those poorly formatted PR descriptions:

  • Some PRs still write single line in PR description with 500+- code changes in a critical path.
  • Some PRs do not describe behaviour changes and reviewers need to find and document.
  • Some PRs are hard to review without outlines but they are not mentioned sometimes.
  • Spark is being old and sometimes we need to track the history deep. Due to poorly formatted PR description, sometimes it requires to read whole codes of whole commit histories to find the root cause of a bug.
  • Reviews take a while but the number of PR still grows.

This PR targets to alleviate the problems and situation.

Does this PR introduce any user-facing change?

Yes, it changes the PR templates when PRs are open. This PR uses the template this PR proposes.

How was this patch tested?

Manually tested via Github preview feature.

@HyukjinKwon
Copy link
Member Author

cc @apache/spark-committers

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@tgravescs
Copy link
Contributor

so I think this brings up a good question as to what we want in the jira and what we want in the PR. Many times they have the same information. Do we want it to be copied to both places, referenced, etc.?

@srowen
Copy link
Member

srowen commented Jul 31, 2019

This is covered in the contributing guide, though that's a big doc. It could be worth a note here that contributors should pay attention to the relationship between JIRAs and PRs as described there. Though I suppose they should really read it all.

@HyukjinKwon
Copy link
Member Author

I think basically JIRA describes what issue and PR describes how it is fixed.

Each focuses on each's purpose but other information can be duplicated for better and faster reviews. I think it's case-by-case.

@HyukjinKwon
Copy link
Member Author

Let me cc some more active contributors who didn't put input here for better visibility and to collect more feedback. cc @viirya @mgaido91 @MaxGekk @HeartSaVioR @gaborgsomogyi @dilipbiswal

@viirya
Copy link
Member

viirya commented Aug 1, 2019 via email

Copy link
Contributor

@mgaido91 mgaido91 left a comment

Choose a reason for hiding this comment

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

thanks for pinging me @HyukjinKwon

@SparkQA

This comment has been minimized.

@HyukjinKwon HyukjinKwon force-pushed the SPARK-28578 branch 2 times, most recently from abe9701 to 3b675ff Compare August 3, 2019 15:03
Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @HyukjinKwon ! I just had some minor grammatical suggestions.

@HyukjinKwon
Copy link
Member Author

I was stuck in some works. Let me update it tomorrow in KST.

@HyukjinKwon
Copy link
Member Author

I address the comments. Should be ready for a look.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@gaborgsomogyi
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Aug 13, 2019

Test build #109031 has finished for PR 25310 at commit 5ac58c9.

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

Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

LGTM

@HyukjinKwon
Copy link
Member Author

Let me merge this one in few days if there are no more comments.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

LGTM

@HyukjinKwon
Copy link
Member Author

Merged to master.

@HyukjinKwon
Copy link
Member Author

Let's see how it goes. thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.