GH-33659: [Developer Tools] Add definition of Breaking Change and Critical Fix #33660
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,3 +255,41 @@ Social aspects | |
| * Like any communication, code reviews are governed by the Apache | ||
| `Code of Conduct <https://www.apache.org/foundation/policies/conduct.html>`_. | ||
| This applies to both reviewers and contributors. | ||
|
|
||
|
|
||
| Labelling | ||
| ========= | ||
|
|
||
| While reviewing PRs, we should try to identify whether these changes need to be | ||
| marked with one or both of the following labels: | ||
|
|
||
| * **Critical Fix**: The change fixes either a security vulnerability or a bug | ||
| that caused incorrect or invalid data to be produced. This is intended to mark | ||
| 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 bugs are | ||
| usually obvious. | ||
| * **Breaking Change**: The change breaks backwards compatibility in a public API. | ||
| For changes in C++, this does not include changes that simply break ABI | ||
| compatibility, except for the few places where we do guarantee ABI | ||
|
Comment on lines
+274
to
+275
Member
There was a problem hiding this comment. Why do we not include ABI compatibility? BTW, we will be able to check ABI compatibility automatically by CI. (Some projects do it.) So we may not check ABI compatibility manually.
Member
Author
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
Member
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.
Member
Author
There was a problem hiding this comment.
I suspect this won't be viable until we do #15280, but worth discussing after. |
||
| compatibility (such as C Data Interface). Experimental APIs are *not* | ||
| exempt from this; they are just more likely to be associated with this tag. | ||
|
|
||
| Breaking changes and critical fixes are separate: breaking changes alter the | ||
| API contract, while critical fixes make the implementation align with the | ||
| existing API contract. For example, fixing a bug that caused a Parquet reader | ||
| to skip rows containing the number 42 is a critical fix but not a breaking change, | ||
| since the row skipping wasn't behavior a reasonable user would rely on. | ||
|
|
||
| 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. | ||
|
Member
There was a problem hiding this comment. This is a nice description, very clear! |
||
|
|
||
| In addition, we use the following labels to indicate priority: | ||
|
|
||
| * **Priority: Blocker**: Indicates the PR **must** be merged before the next | ||
|
Member
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:
Member
Author
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.
Member
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) |
||
| release can happen. This includes fixes to test or packaging failures that | ||
| would prevent the release from succeeding final packaging or verification. | ||
| * **Priority: Critical**: Indicates issues that are high priority. This is a | ||
| superset of issues marked "Critical Fix", as it also contains certain fixes | ||
| to issues causing errors and crashes. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatadd(2, 2)returned5, fixing that to return4is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wanted, we could consider a separate tag for
Experimental API Change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is very helpful, let's include some of it.
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.