GH-33659: [Developer Tools] Add definition of Breaking Change and Critical Fix #33660
Conversation
.github/pull_request_template.md
Outdated
| <!-- | ||
| If there are any breaking changes to public APIs, please add the `breaking-change` label. | ||
| --> | ||
| If there are any breaking changes to public APIs, please uncomment the following line: |
There was a problem hiding this comment.
I have seen projects use markdown check boxes in pr templates for things like these and other "checklist" item like tests etc... might be a good use case here too?
There was a problem hiding this comment.
I will look into that.
|
Thanks I really like this! A needed step into the direction of properly versioning each component independently! |
| <!-- | ||
| If the changes fix either a security vulnerability or a bug that caused incorrect or invalid data to be produced. This is intended to mark changes that may affect users without their knowledge. For this reason, fixing bugs that cause errors or crashes don't count, since those are usually obvious. | ||
| --> |
There was a problem hiding this comment.
This is a little confusing to me.
If we were previously giving the wrong answer, and we fixed something to give a new answer, would I mark it as both breaking change and critical fix?
Also, hopefully most security fixes would go unnoticed by a user. I wouldn't expect them (by default) to change the output or the API.
Also, do we want to carve out an exception for experimental APIs?
There was a problem hiding this comment.
Hmm I guess I should clarify since Rok had the same question. The difference is "Breaking Change" is when we change our promise about how an API behaves, while "Critical Fix" changes the implementation to align with the existing promise. For example, if we have a function add(x, y) that promised to add two numbers, but we found that add(2, 2) returned 5, fixing that to return 4 is a Critical Fix, not a breaking change. In the Arrow project, this typically shows up in file readers and writers, where we sometimes parse or serialize data incorrectly. Fixing those bugs don't really constitute breaking changes IMO.
Another way to put it is "Breaking Changes" are the reasons you might not want to upgrade until you have time to adapt your code, while "Critical Changes" highlight the risk of not upgrading.
There was a problem hiding this comment.
Also, do we want to carve out an exception for experimental APIs?
And that's a good question on experimental APIs... I'm inclined to say it's still good to mark them as breaking changes. When we mark them as experimental, we are telling users to expect the API to change, but not that we won't warn them if they do 😄. That being said, in the future if we decide to do semantic versioning, we would probably treat breaking changes in experimental APIs differently from those in stable APIs.
There was a problem hiding this comment.
If we wanted, we could consider a separate tag for Experimental API Change.
There was a problem hiding this comment.
Another way to put it is "Breaking Changes" are the reasons you might not want to upgrade until you have time to adapt your code, while "Critical Changes" highlight the risk of not upgrading.
This description is very helpful, let's include some of it.
If we wanted, we could consider a separate tag for Experimental API Change.
No, I think your rationale is ok. Even in our experimental APIs we usually avoid breaking changes if we can so it isn't like they occur all that often.
.github/pull_request_template.md
Outdated
| <!-- | ||
| Check the box below if the changes fix either a security vulnerability or a bug that caused incorrect or invalid data to be produced. We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors or crashes don't count, since those are usually obvious. | ||
| --> | ||
| * [ ] This PR contains a "Critical Fix". |
There was a problem hiding this comment.
Minor argument against using checkboxes for this: this adds a "todo" list to each PR, and then the GitHub UI will show this for example as "1 of 2 tasks" done in the pull request listing
(ideally github would also allow you use a "form" and not just a template, so this could be an actual checkbox that would add the label)
There was a problem hiding this comment.
Also, if there are breaking changes to the public API, we should maybe ask to actually list / describe the exact change(s), instead of just checking the box? (and potentially give some rationale for making the change, although that could also refer to the issue)
There was a problem hiding this comment.
(ideally github would also allow you use a "form" and not just a template, so this could be an actual checkbox that would add the label)
Yeah I think to get close to that we could have our bot add the labels if these boxes are checked (or using some other indicator).
There was a problem hiding this comment.
Based on your feedback, I've reverted the instructions to "uncomment the line below" and also requested the creator provide explanation on which specific changes are breaking or critical fix.
| These labels are used in the release to highlight changes that users ought to be | ||
| aware of when they consider upgrading library versions. Breaking changes help | ||
| identify reasons when users may wish to wait to upgrade until they have time to | ||
| adapt their code, while critical fixes highlight the risk in *not* upgrading. |
There was a problem hiding this comment.
This is a nice description, very clear!
docs/source/developers/reviewing.rst
Outdated
|
|
||
| In addition, we use the following labels to indicate priority: | ||
|
|
||
| * **Priority: Blocker**: Indicates the PR **must** be merged before the next |
There was a problem hiding this comment.
At the moment for our release tasks we use the labels only on issues, not on PRs. Here we refer to labels on PRs and issues without distinction: Indicates issues that are high priority.. We also enforce the update of the milestone only on issues when merging a PR, not on PRs.
Right now the commit title on the merge script allows us to link to the PR because we use the following: commit_title = f'{self.title} (#{self.number})' which links both the issue GH-XXXX and the PR (#YYYYY) even though at the moment we don't use the PR on our release tasks, we could.
I suppose my concern is around the workflow changes and whether we want to force those labels only on issues, only on PRs, on both, what happens if a PR and an issue contain different types of labels, etcetera.
This might affect the discussion about removing the merge script or at least how we link issues with PRs.
There was a problem hiding this comment.
Thanks for pointing this out. I should change to say these labels should be applied to the issue, not the PR.
There was a problem hiding this comment.
Personally, as a reviewer, I would find it useful to have those labels on the PR as well ..
(but don't let this drag on this PR. It's a general issue we have right now with labeling and milestoning issues vs PRs)
Co-authored-by: Raúl Cumplido <[email protected]>
| For changes in C++, this does not include changes that simply break ABI | ||
| compatibility, except for the few places where we do guarantee ABI |
There was a problem hiding this comment.
Why do we not include ABI compatibility?
I think that we need to bump major version when we break API or ABI compatibility.
I thought that we use "Breaking Change" to determine whether we should bump major/minor version.
BTW, we will be able to check ABI compatibility automatically by CI. (Some projects do it.) So we may not check ABI compatibility manually.
There was a problem hiding this comment.
What I meant is that we don't guarantee that your code compiled against the headers of Arrow 10.0.0 can be linked against the Arrow 11.0.0 shared libraries. But we do try to keep it stable such that you don't need to change your code when you upgrade the dependency.
For example, IIUC changing a function signature from
void MyFunction(int arg1);To:
void MyFunction(int arg1, int arg2 = 0);Breaks ABI compatibility, since it alters the symbol for MyFunction in the shared library. But downstream projects won't have to change their code when they upgrade. So this paragraph means we wouldn't label this change as a "Breaking Change".
There was a problem hiding this comment.
Thanks. I see.
My understanding: We use "Breaking Change" to provide upgrade difficulty for users. We don't use "Breaking Change" to determine which version component (major/minor/patch) should be bumped.
There was a problem hiding this comment.
We don't use "Breaking Change" to determine which version component (major/minor/patch) should be bumped.
I suspect this won't be viable until we do #15280, but worth discussing after.
|
Benchmark runs are scheduled for baseline = 3b3afc0 and contender = 247c2e6. 247c2e6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
7 participants
Uh oh!
There was an error while loading. Please reload this page.