-
Notifications
You must be signed in to change notification settings - Fork 292
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
chore: revert opendal 0.46 #4098
Conversation
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.
Could you elaborate what is the issue introduced by OpenDAL 0.46?
Generally we fix issues instead of reverting. Unless there is an emergency to release or it's the dependency's fault that the new version contains bugs. A PR without a description to revert commits looks confusing. |
We need to explain why we're doing this. @WenyXu |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4098 +/- ##
==========================================
- Coverage 85.47% 85.21% -0.27%
==========================================
Files 991 991
Lines 173053 172997 -56
==========================================
- Hits 147920 147420 -500
- Misses 25133 25577 +444 |
Yes, I will try to remove the |
Hi, seems better to adapt new API instead of reverting? |
Would you like to raise a bug for this? |
Yes, I will adapt it by simplifying/removing the
We found this bug while investigating another bug, I'm still trying to figure out why it happens 😢 |
The OOM maybe related to https://opendal.apache.org/docs/rust/opendal/docs/rfcs/rfc_4638_executor/index.html |
I think the key problem here is that |
Oops, I reviewed the code; the chunk size will be set automatically via the service's capability attributes. There must be something else I don't know🥹 |
Only set while chunk has been set. Write will go directly if chunk is None. |
Oh, I see. So, the key problem is that we didn't set the chunk size.🥲 |
Yep, it's quite easy to be used in wrong. I'm considering change the behavior here: we can make the min_write_size as the default chunk size. If users has chunk set, we will respect users input and pick up max(chunk, min_write_size). All requests will be the same chunk size. If users don't have chunk set, we will use min_write_size as chunk size. But we won't really perform the chunk operation: all write that larger than min_write_size will be sent directly. Would you like to start a pr for this? |
Should we release version 0.8.2? |
Another topic is that we're currently mixing up bug fixes and feature development in patch releases as we are in the 0.x series. Once we release 1.0, perhaps we should use multi branches and only backport bug fixes to patch releases. The feature releases (minor or major) would have a release train model so that developers know clear the timeline and when to include changes and when feature freeze and test. cc @fengjiachun |
Agree, I think we must set up the release rules after 1.0 |
It seems we can't reuse the |
Sorry for mixing the context. I'm talking about sending PRs to opendal for changing the behavior. |
fixed #4100 |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#2662
What's changed and what's your intention?
Revert opendal 0.46
After upgrading to opendal 0.46, it incurs the
EntityTooSmall
error during closing the writer in the flush/compaction job.The opendal's
FuturesAsyncWriter
doesn't work well with ourBufferedWriter
🥲. Maybe we should use thechunk
method provided by opendal instead of implementing aBufferedWriter
.i.e.
Checklist