You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/06 22:36:27 UTC
[GitHub] [incubator-pinot] mcvsubbu opened a new pull request #5345: Create PULL_REQUEST_TEMPLATE.md
mcvsubbu opened a new pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345
Added a pull request template so that PRs can be labelled appropriately
While creating release notes, it is hard to find which PRs need special mention because of new features, backward incompatibility, upgrade order, new configurations, etc. This PR creates a template that we can use for this purpose.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#discussion_r422412379
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
+Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
+* [ ] Yes (Please label as **<code>backward-incompat</code>**)
+
+Does this PR fix a zero-downtime upgrade introduced earlier?
+* [ ] Yes (Please label this as **<code>backward-incompat</code>**)
Review comment:
This bullet is not very intuitive to me. Are we saying that if you broke zero downtime upgrade earlier and now fixing it as part of this PR then label it backward incompat? That should be backward compatible. We recently had this scenario.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] haibow commented on a change in pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
haibow commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#discussion_r422594460
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
+Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
+* [ ] Yes (Please label as **<code>backward-incompat</code>**)
+
+Does this PR fix a zero-downtime upgrade introduced earlier?
+* [ ] Yes (Please label this as **<code>backward-incompat</code>**)
+
+Does this PR otherwise need attention when creating release notes? Things to consider:
+- New configuration options
+- Deprecation of configurations
+- Signature changes to public methods/interfaces
+- New plugins added or old plugins removed
+* [ ] Yes (Please label this PR as **<code>release-notes</code>**)
Review comment:
Even with the "Things to consider", the release notes section is still just a Yes/No question. When the answer is Yes, better explicitly ask the author to prepare some release notes, to make the release master's life easier.
e.g. (borrowing the norm from Presto)
```
== RELEASE NOTES ==
General Changes
* ...
* ...
```
If release note is NOT required, use:
```
== NO RELEASE NOTE ==
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#issuecomment-625377921
> This template is great!
> Better to add a section for plugins related changes.
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#discussion_r422304502
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
+Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
+* [ ] Yes (Please label as **<code>backward-incompat</code>**)
+
+Does this PR fix a zero-downtime upgrade introduced earlier?
Review comment:
whats the purpose of this question?
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
Review comment:
Please add a description section.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu merged pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#issuecomment-624966799
This template is great!
Better to add a section for plugins related changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#discussion_r422568206
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
Review comment:
I had the before, but the problem is that when someone creates a PR, the description shows up _before_ the Description section. I can add it if that is blocking merge of this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#issuecomment-631133291
whats the next step here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#issuecomment-631136749
I need some concrete suggestion from @siddharthteotia and approval, since he had comments. We can iterate on this, later
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5345: Create PULL_REQUEST_TEMPLATE.md
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-pinot/pull/5345#discussion_r422568460
##########
File path: .github/PULL_REQUEST_TEMPLATE.md
##########
@@ -0,0 +1,13 @@
+## Upgrade Notes
+Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
+* [ ] Yes (Please label as **<code>backward-incompat</code>**)
+
+Does this PR fix a zero-downtime upgrade introduced earlier?
Review comment:
1. LinkedIn pulls weekly from open source and deploys into production. LinkedIn (and others who may pull regularly from open source pinot) can get some information of what may break the deployment (even if retro-fixed).
2. It is a good practice to have release notes that describe the upgrade scenarios that may break. If we have good labels on PRs, whoever cuts the release can compile the release notes easily.
3. It hopefully makes the reviewers think whether something may break compatibility.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org