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.

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

feat: Add OpenDAL Compat #5185

Merged
merged 6 commits into from
Oct 16, 2024
Merged

feat: Add OpenDAL Compat #5185

merged 6 commits into from
Oct 16, 2024

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Oct 15, 2024

Which issue does this PR close?

Closes #5180.

Rationale for this change

Make users' lives easier when they need to work with different versions of OpenDAL.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Copy link
Member

@morristai morristai left a comment

Choose a reason for hiding this comment

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

LGTM : )

@yuchanns
Copy link
Member

yuchanns commented Oct 16, 2024

IMO the old operator should be tested against the previous tests , not current tests.

For example, #4959 has modified the related tests. In this case, it's impossible for the old operator to pass the new tests. (If I cited the wrong case, please remind me.)

While this situation is relatively uncommon, I believe it’s one of the aspects you aim to address by introducing this crate.

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 16, 2024

IMO the old operator should be tested against the previous tests , not current tests.

After using compat, our behavior will align with the new operator instead. We cannot maintain the old behavior as it currently stands.

For example, #4959 has modified the related tests. In this case, it's impossible for the old operator to pass the new tests. (If I cited the wrong case, please remind me.)

Unfortunately, we are unable to implement this. Users will still need to be mindful of the behavioral changes.

@yuchanns
Copy link
Member

OK. So the compat feature doesn't guarantee the behavior is compatible after the upgrade. It is sad.

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 16, 2024

OK. So the compat feature doesn't guarantee the behavior is compatible after the upgrade. It is sad.

Yes, just ensure that the application can continue building. The maintenance burden is too high for us to ensure behavior compatibility as well.

I will make it clear in our docs.

Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 16, 2024

CC @yuchanns, please review 8071f91 (#5185) and let me know if it looks good to you.

@yuchanns
Copy link
Member

yuchanns commented Oct 16, 2024

LGTM on green

@Xuanwo Xuanwo merged commit 9d0f715 into main Oct 16, 2024
278 of 279 checks passed
@Xuanwo Xuanwo deleted the opendal_compat branch October 16, 2024 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants