-
Notifications
You must be signed in to change notification settings - Fork 333
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
build(deps): upgrade opendal to 0.46 #4037
Conversation
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Hi, in tisonkun#5, I make greptime buildable with opendal 0.46. We still have more work to do moving forward. For example, we need to reconsider the use of Also, since opendal 0.46, it returns There are some places for opendal to improve like |
Signed-off-by: Xuanwo <[email protected]>
For PrometheusLayer, I raised #4041 |
Signed-off-by: tison <[email protected]>
@Xuanwo seems we have two test cases failed:
The former seems related to ORC read content, the latter seems related to range encoding. Would you help take a look? |
cache test should be fixed by tisonkun#6 Note, there is no |
Signed-off-by: tison <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Thank you and integrated. Could you cross-link the OpenDAL PR that can fix the hack you mention in tisonkun#6? |
It happens in apache/opendal#4634 |
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4037 +/- ##
==========================================
- Coverage 85.43% 85.11% -0.33%
==========================================
Files 985 985
Lines 170515 170571 +56
==========================================
- Hits 145687 145182 -505
- Misses 24828 25389 +561 |
It seems that there are a few tasks waiting for the opendal 0.4.7 release. I believe we should postpone this PR until then. |
@Xuanwo Is this regression from 0.45 to 0.46? If not, I'd prefer we upgrade to 0.46, and then 0.47 as it's expected to release in the next few weeks: |
No, I don't think it's a regression. The current behavior is expected. OpenDAL 0.47 simply introduces some additional improvements, for example, allow users calling |
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.
@Xuanwo Thanks for helping us to upgrade OpenDAL. I left some non-blocking comments to remind us after we merge this PR.
Signed-off-by: tison <[email protected]>
@killme2008 as this won't introduce explicit regression. I'd prefer we upgrade to 0.46, and then 0.47 as it's expected to release in the next few weeks. See #4037 (comment). |
Signed-off-by: tison <[email protected]>
By the way, I'm fixing the failed CI in #4049.🥲 |
Signed-off-by: tison <[email protected]>
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.
LGTM
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Checklist