Skip to content
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

chore: Migration Guide #13849

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: Migration Guide #13849

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3525a94
chore: Migration Guide
comphead Dec 19, 2024
74351d3
chore: Migration Guide
comphead Dec 19, 2024
c967134
chore: Migration Guide
comphead Dec 19, 2024
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
20 changes: 20 additions & 0 deletions MIGRATION_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<!---
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->


1 change: 1 addition & 0 deletions docs/source/contributor-guide/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ Some things to specifically check:

1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)?
2. Is the code clear, and fits the style of the existing codebase?
3. Does the PR introduce a [breaking change](https://datafusion.apache.org/library-user-guide/api-health.html#breaking-changes)? If so the `api_change` Github badge needs to be added to the PR and migration documentation need to reflect changes according to [Migration Guidelines](https://datafusion.apache.org/library-user-guide/api-health.html#migration-guidelines)

## Performance Improvements

Expand Down
29 changes: 29 additions & 0 deletions docs/source/library-user-guide/api-health.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,32 @@ For example:
Deprecated methods will remain in the codebase for a period of 6 major versions or 6 months, whichever is longer, to provide users ample time to transition away from them.

Please refer to [DataFusion releases](https://crates.io/crates/datafusion/versions) to plan ahead API migration

## Migration Guidelines

To ensure smooth upgrades and maintain application stability, the following guidelines must be followed for changes involving:
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistically I would prefer to say this more gently "the following guidelines should be followeed" rather than must. Mostly because there is no way for us to enforce this policy other than pull request reviews

However, I think that may just be a cultural hangup I have as I come from the US and the distinction may not be relevant for other contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can also explicitly say here "it is preferrable not to make breaking API changes and instead add new APis and deprecate existing ones in which case no migration guide is needed"

That would perhaps also subtley incentivize people to avoid breaking changes 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we covered it in https://datafusion.apache.org/library-user-guide/api-health.html#breaking-changes ?


- Public API changes
Copy link
Member

Choose a reason for hiding this comment

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

we should say what consistutes a public api change

  • removal of a public function
  • signature change of a public function
  • semantic changes if a public funciton
  • new method in a public trait that has no default impl
  • new required traits of a public trait (eg requiring a Foo to be Sync)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is good point.

- Introducing deprecated methods
Copy link
Member

Choose a reason for hiding this comment

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

This should not involve any additional migration guide. I.e. this is automatic (unless the deprecated method is no longer correct to call; in such case we should remove it instead of deprecating, or fix it)

- Removal of obsolete methods
Comment on lines +78 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't think the last two items are worth documenting -- deprecated methods should have already been removed so there is no need for a contributor to try and document anything when removing them

I would also like us to get more in the habit of deprecating rather than removing methods which will make the last item irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated functions are self explanatory if deprecation guidelines followed :)

But one pros for documenting it in migration plan is the user is able to identify amount of migration work needed just by looking into the guide for a specific version.

For example https://spark.apache.org/docs/3.5.3/core-migration-guide.html#upgrading-from-core-34-to-35

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't think the last two items are worth documenting

agreed

But one pros for documenting it in migration plan is the user is able to identify amount of migration work needed just by looking into the guide for a specific version.

otoh it adds burden for development, and sets wrong incentives, discouraging from deprecating methods and removing old code

If this is indeed useful, let's maybe automate pulling of new deprecations, rather than impose a new tax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automatic deprecation doc generation sounds good. We can think of it later.
I'm not sure about removing it completely as it tracks API evolution for the user.

There would be always a balance between development work and migration support. From recent days we understood user migration is important for them


Highlight all breaking changes, deprecated methods, and obsolete methods in the [migration guide](../../../MIGRATION_GUIDE.md).

### Migration Document Requirements:

For each upgrade, append a section in [migration guide](../../../MIGRATION_GUIDE.md) outlining the following:
Copy link
Member

Choose a reason for hiding this comment

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

will we have merge conflicts there often?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course, like any other file, but should we easily resolvable.


#### Breaking Changes:

- Describe all changes that break backward compatibility.
- Provide detailed steps to migrate from old to new APIs.

#### Deprecations:

- List all deprecated methods and expected removal timelines.
- Provide alternative methods or workarounds.

#### Removals:

- List all methods or APIs that are removed in the current release.
- Suggest migration paths for impacted functionalities
Comment on lines +94 to +100
Copy link
Member

Choose a reason for hiding this comment

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

ouch, unless automated, we won't deprecate any code and won't remove and obsolete stuff.
let's not do this

Loading