Skip to content

GH-33659: [Developer Tools] Add definition of Breaking Change and Critical Fix #33660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wjones127 merged 9 commits into from
Jan 25, 2023
Merged

GH-33659: [Developer Tools] Add definition of Breaking Change and Critical Fix #33660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b2ab856
Add definition of Breaking Change and Critical Fix
wjones127 Jan 13, 2023
ad0afea
use a checkbox
wjones127 Jan 13, 2023
f3dc08e
use checkboxes and explain priority labels
wjones127 Jan 13, 2023
982b03d
Add more explanation
wjones127 Jan 13, 2023
8adde88
note that experimental APIs aren't exempt
wjones127 Jan 13, 2023
af3d726
update definition of critical fix to include crashes
wjones127 Jan 17, 2023
a0f7db5
add better mechanism and request explanation
wjones127 Jan 17, 2023
21dac1a
Update .github/pull_request_template.md
wjones127 Jan 18, 2023
6729725
clarify to use issue not PR labels
wjones127 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,11 @@ If there are user-facing changes then we may require documentation to be updated
-->

<!--
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 line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->

<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
40 changes: 40 additions & 0 deletions docs/source/developers/reviewing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,43 @@ 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 the corresponding issue
needs to be marked with one or both of the following issue labels:

* **Critical Fix**: The change fixes either: (a) a security vulnerability;
(b) a bug that causes incorrect or invalid data to be produced;
or (c) a bug that causes a crash (while the API contract is upheld).
This is intended to mark fixes to issues that may affect users without their
knowledge. For this reason, fixing bugs that cause errors don't count, since
those bugs are usually obvious. Bugs that cause crashes are considered critical
because they are a possible vector of Denial-of-Service attacks.
* **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
Copy link
Member

@kou kou Jan 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@wjones127 wjones127 Jan 19, 2023

Choose a reason for hiding this comment

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

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".

Copy link
Member

@kou kou Jan 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@wjones127 wjones127 Jan 19, 2023

Choose a reason for hiding this comment

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

For now, yes.

Copy link
Member Author

@wjones127 wjones127 Jan 19, 2023

Choose a reason for hiding this comment

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

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.

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.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 16, 2023

Choose a reason for hiding this comment

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

This is a nice description, very clear!


In addition, we use the following labels to indicate priority:

* **Priority: Blocker**: Indicates the changes **must** be merged before the next
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.