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

build(deps): upgrade opendal to 0.46 #4037

Merged
merged 20 commits into from
May 27, 2024
Merged

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented May 24, 2024

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?

  1. Upgrade OpenDAL to 0.46.
  2. Upgrade reqwest to 0.12 cascading.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.

@tisonkun tisonkun requested a review from a team as a code owner May 24, 2024 14:15
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 24, 2024
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]>
@Xuanwo
Copy link
Contributor

Xuanwo commented May 25, 2024

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 AsyncRead + AsyncSeek in our codebase, as they are not suitable for object storage services and upcoming completion-based IO.

Also, since opendal 0.46, it returns Buffer which provide zero-copy of data. We can try use this feature instead of calling to_vec, to_bytes everywhere. For example, our decoder can accept std::io::Read instead of &[u8].

There are some places for opendal to improve like into_futures_async_read requires Range<u64>. We have fixed it and will be included in next release.

@Xuanwo
Copy link
Contributor

Xuanwo commented May 25, 2024

For PrometheusLayer, I raised #4041

Signed-off-by: tison <[email protected]>
@tisonkun
Copy link
Contributor Author

@Xuanwo seems we have two test cases failed:

  • test_orc_opener - Footer length is empty
  • test_object_store_cache_policy - file not found: ecfe0dce85de452eb0a325158e7bfb75.cache-bytes=7-4194310

The former seems related to ORC read content, the latter seems related to range encoding.

Would you help take a look?

@Xuanwo
Copy link
Contributor

Xuanwo commented May 25, 2024

cache test should be fixed by tisonkun#6

Note, there is no 0.. read anymore.

Signed-off-by: tison <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@tisonkun
Copy link
Contributor Author

@Xuanwo Thank you and integrated. Could you cross-link the OpenDAL PR that can fix the hack you mention in tisonkun#6?

@Xuanwo
Copy link
Contributor

Xuanwo commented May 25, 2024

@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]>
Copy link

codecov bot commented May 25, 2024

Codecov Report

Attention: Patch coverage is 83.68580% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 85.11%. Comparing base (20ce7d4) to head (f9d5f78).

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     

@killme2008
Copy link
Contributor

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.

@tisonkun
Copy link
Contributor Author

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:

@Xuanwo
Copy link
Contributor

Xuanwo commented May 27, 2024

Is this regression from 0.45 to 0.46?

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 into_futures_async_read with size.

Copy link
Contributor

@evenyag evenyag left a 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.

src/datanode/src/store.rs Outdated Show resolved Hide resolved
src/datanode/src/store.rs Outdated Show resolved Hide resolved
src/mito2/src/cache/write_cache.rs Show resolved Hide resolved
src/mito2/src/manifest/storage.rs Outdated Show resolved Hide resolved
src/mito2/src/sst/index/applier.rs Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor Author

@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]>
@tisonkun tisonkun requested a review from WenyXu May 27, 2024 06:06
@WenyXu
Copy link
Member

WenyXu commented May 27, 2024

By the way, I'm fixing the failed CI in #4049.🥲

src/datanode/src/store.rs Outdated Show resolved Hide resolved
@WenyXu WenyXu requested review from zhongzc and killme2008 May 27, 2024 06:30
@tisonkun tisonkun enabled auto-merge May 27, 2024 09:02
Copy link
Member

@WenyXu WenyXu left a comment

Choose a reason for hiding this comment

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

LGTM

@tisonkun tisonkun added this pull request to the merge queue May 27, 2024
Merged via the queue into GreptimeTeam:main with commit f9db5ff May 27, 2024
28 checks passed
@tisonkun tisonkun deleted the opendal-0460 branch May 27, 2024 09:28
waynexia added a commit that referenced this pull request Jun 3, 2024
@killme2008 killme2008 mentioned this pull request Jun 3, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants