refactor: use usize range instead of BlockLocation to read obj #12225

MrCroxx · 2023-09-12T06:53:13Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Use impl RangeBounds<usize> + Clone + Sync + Send + 'static to replace BlockLocation.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: MrCroxx <[email protected]>

codecov · 2023-09-12T07:25:39Z

Codecov Report

Merging #12225 (974f75a) into main (62901e1) will decrease coverage by 0.01%.
The diff coverage is 61.29%.

@@            Coverage Diff             @@
##             main   #12225      +/-   ##
==========================================
- Coverage   69.67%   69.66%   -0.01%     
==========================================
  Files        1410     1411       +1     
  Lines      236255   236127     -128     
==========================================
- Hits       164599   164505      -94     
+ Misses      71656    71622      -34

Flag	Coverage Δ
rust	`69.66% <61.29%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/ctl/src/cmd_impl/hummock/sst_dump.rs	`0.00% <0.00%> (ø)`
src/object_store/src/object/s3.rs	`3.15% <0.00%> (+0.07%)`	⬆️
src/storage/src/hummock/sstable_store.rs	`68.59% <42.85%> (+0.44%)`	⬆️
src/common/src/range.rs	`62.16% <62.16%> (ø)`
src/object_store/src/object/mem.rs	`92.06% <95.00%> (+3.52%)`	⬆️
src/meta/src/hummock/manager/checkpoint.rs	`75.18% <100.00%> (ø)`
src/meta/src/hummock/manager/mod.rs	`60.84% <100.00%> (-0.20%)`	⬇️
src/object_store/src/object/mod.rs	`59.02% <100.00%> (+4.70%)`	⬆️
.../src/object/opendal_engine/opendal_object_store.rs	`73.51% <100.00%> (-5.24%)`	⬇️
src/storage/backup/src/storage.rs	`54.95% <100.00%> (ø)`
... and 1 more

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Li0k · 2023-09-12T08:02:10Z

src/object_store/src/object/mem.rs

+    async fn get_object(
+        &self,
+        path: &str,
+        range: impl RangeBounds<usize> + Clone + Send + Sync + 'static,


src/storage/src/store.rs StaticSendSync

range here requires a lot more trait bounds, IMO, expend them here is also okay.

Li0k

Rest LGTM, thanks

Li0k · 2023-09-12T08:03:24Z

src/ctl/src/cmd_impl/hummock/sst_dump.rs

+            // version, magic
+            2 * std::mem::size_of::<u32>() +
+        // footer, checksum
+        2 * std::mem::size_of::<u64>()


src/object_store/src/object/mem.rs

src/object_store/src/object/s3.rs

Li0k · 2023-09-12T08:07:08Z

src/object_store/src/object/s3.rs

        }
+
+        let start = match range.start_bound() {


How about constructing start and end in separate functions? It's repeated several times in pr

Agreed! I'm doing it.

hzxa21 · 2023-09-12T07:24:58Z

src/object_store/src/object/mem.rs

+        let start = match range.start_bound() {
+            std::ops::Bound::Included(v) => *v,
+            std::ops::Bound::Excluded(v) => *v - 1,
+            std::ops::Bound::Unbounded => 0,
+        };
+        let end = match range.end_bound() {
+            std::ops::Bound::Included(v) => *v + 1,
+            std::ops::Bound::Excluded(v) => *v,
+            std::ops::Bound::Unbounded => obj.len(),
+        };
+
+        if end > obj.len() {
+            return Err(Error::other("bad block offset and size").into());
+        }

-fn find_block(obj: &Bytes, block: BlockLocation) -> ObjectResult<Bytes> {
-    if block.offset + block.size > obj.len() {
-        Err(Error::other("bad block offset and size").into())
-    } else {
-        Ok(obj.slice(block.offset..(block.offset + block.size)))
+        Ok(obj.slice(start..end))


Why not passng the range directly to obj.slice?

Code here needs to check overflow.

hzxa21 · 2023-09-12T08:58:23Z

src/object_store/src/object/s3.rs

+
+        let start = match range.start_bound() {
+            std::ops::Bound::Included(v) => v.to_string(),
+            std::ops::Bound::Excluded(v) => (v - 1).to_string(),


Shouldn't this be v+1?

We may need some tests to test different combinations of left close/open/unbounded and right close/open/unbounded. If it is hard to mock aws client, we can mannually test it as well.

Yep! I've found it and fixed it. (I thought no one find it and I can fix it quietly. 🤣)

We may need some tests to test different combinations of left close/open/unbounded and right close/open/unbounded. If it is hard to mock aws client, we can mannually test it as well.

After the latest commits, combinations of bounds are implemented in RangeBoundsExt.

Signed-off-by: MrCroxx <[email protected]>

hzxa21

LGTM

Signed-off-by: MrCroxx <[email protected]>

wenym1 · 2023-09-12T09:43:36Z

src/common/src/range.rs

+    + 'static
+    + ZeroOne;
+
+pub trait RangeBoundsExt<T>


RangeBoundsExt<T>: RangeBounds<T>?

wenym1 · 2023-09-12T09:47:34Z

src/object_store/src/object/mem.rs

+    async fn read(
+        &self,
+        path: &str,
+        range: impl RangeBounds<usize> + Clone + Send + Sync + std::fmt::Debug + 'static,


May define pub trait ObjectStoreRange = RangeBounds<usize> + Clone + Send + Sync + std::fmt::Debug + 'static to avoid defining the long trait for multiple times.

wenym1 · 2023-09-12T09:51:22Z

src/common/src/range.rs

+
+mod private {
+
+    pub trait ZeroOne {


Found a crate that implements similar functionality.

https://docs.rs/num/latest/num/traits/trait.Zero.html

Yep, there is One and Zero, but I need both of them. 🤣 I preferred not to introduce two more dependencies here.

Signed-off-by: MrCroxx <[email protected]>

refactor: use usize range instead of BlockLocation to read obj

cb24ac6

Signed-off-by: MrCroxx <[email protected]>

MrCroxx requested review from hzxa21, wenym1 and wcy-fdu September 12, 2023 06:53

MrCroxx self-assigned this Sep 12, 2023

github-actions bot added the type/refactor label Sep 12, 2023

MrCroxx added 2 commits September 12, 2023 07:01

check range get sizee

dcf7b97

Signed-off-by: MrCroxx <[email protected]>

Merge branch 'main' into xx/refactor-obj-range

8969e5b

Li0k reviewed Sep 12, 2023

View reviewed changes

Li0k approved these changes Sep 12, 2023

View reviewed changes

hzxa21 reviewed Sep 12, 2023

View reviewed changes

introduce RangeBoundsExt to simplify code

a7d9a1f

Signed-off-by: MrCroxx <[email protected]>

hzxa21 approved these changes Sep 12, 2023

View reviewed changes

MrCroxx added 2 commits September 12, 2023 09:40

Merge branch 'main' into xx/refactor-obj-range

8f75b26

fix s3 range header

a73d8cd

Signed-off-by: MrCroxx <[email protected]>

MrCroxx enabled auto-merge September 12, 2023 09:47

wenym1 reviewed Sep 12, 2023

View reviewed changes

MrCroxx disabled auto-merge September 12, 2023 09:48

wenym1 reviewed Sep 12, 2023

View reviewed changes

simplify code

59a1617

Signed-off-by: MrCroxx <[email protected]>

MrCroxx enabled auto-merge September 12, 2023 10:02

Merge branch 'main' into xx/refactor-obj-range

167e1b0

MrCroxx added this pull request to the merge queue Sep 12, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 12, 2023

Merge branch 'main' into xx/refactor-obj-range

974f75a

MrCroxx enabled auto-merge September 12, 2023 11:13

MrCroxx added this pull request to the merge queue Sep 12, 2023

Merged via the queue into main with commit 1650a3b Sep 12, 2023
6 of 7 checks passed

MrCroxx deleted the xx/refactor-obj-range branch September 12, 2023 11:47

Li0k pushed a commit that referenced this pull request Sep 15, 2023

refactor: use usize range instead of BlockLocation to read obj (#12225)

5b7d720

Signed-off-by: MrCroxx <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use usize range instead of BlockLocation to read obj #12225

refactor: use usize range instead of BlockLocation to read obj #12225

MrCroxx commented Sep 12, 2023

codecov bot commented Sep 12, 2023 •

edited

Loading

Li0k Sep 12, 2023

MrCroxx Sep 12, 2023

Li0k left a comment

Li0k Sep 12, 2023

Li0k Sep 12, 2023

MrCroxx Sep 12, 2023

hzxa21 Sep 12, 2023

MrCroxx Sep 12, 2023

hzxa21 Sep 12, 2023

hzxa21 Sep 12, 2023

MrCroxx Sep 12, 2023

hzxa21 left a comment

wenym1 Sep 12, 2023

wenym1 Sep 12, 2023

wenym1 Sep 12, 2023

MrCroxx Sep 12, 2023

refactor: use usize range instead of BlockLocation to read obj #12225

refactor: use usize range instead of BlockLocation to read obj #12225

Conversation

MrCroxx commented Sep 12, 2023

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2023 •

edited

Loading