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

fix: read a large range without error and add test #1068

Merged
merged 5 commits into from
Dec 12, 2022

Conversation

Ranxy
Copy link
Contributor

@Ranxy Ranxy commented Dec 12, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Summary about this PR
Close #943 fix error if block range read with a large range and add test for it.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Mostly LGTM, but I prefer to make a new test case so that we can address them more easier.

tests/behavior/blocking_write.rs Outdated Show resolved Hide resolved
tests/behavior/write.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Dec 12, 2022

[2022-12-12T09:54:23Z DEBUG opendal::services] service=s3 operation=read path=ffbd8eab-4916-4b12-b302-a4a4387b7ada.gz range=0- -> got reader
[2022-12-12T09:54:23Z DEBUG opendal::services] service=s3 operation=read path=d2fba631-9198-49c6-b55f-f64bcfd468c8 range=1139367-18446744073709551614 -> got reader
[2022-12-12T09:54:23Z DEBUG opendal::services] service=s3 operation=read path=d2fba631-9198-49c6-b55f-f64bcfd468c8 has_read=2034952 -> dropped reader
thread 'services_s3_write::test_read_large_range' panicked at 'assertion failed: `(left == right)`
  left: `2034952`,
 right: `895585`: read size with large range', tests/behavior/write.rs:425:5

Interestingly, minio failed this test case. But minio should support range requests. We need more investigating.

Can you try use smaller size like u32::MAX?

@Xuanwo
Copy link
Member

Xuanwo commented Dec 12, 2022

@Ranxy
Copy link
Contributor Author

Ranxy commented Dec 12, 2022

Okay, let me try.

@Ranxy
Copy link
Contributor Author

Ranxy commented Dec 12, 2022

It seems that tests on hdfs are not stable, fortunately, the minio problem is solved.
Can we restart this test separately, Or just push an empty commit to restart all?

@Xuanwo
Copy link
Member

Xuanwo commented Dec 12, 2022

It seems that tests on hdfs are not stable

It's a known issue about hdfs's test: #465

And I have no idea on this, let's just retry it.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks for contribution!

@Xuanwo Xuanwo merged commit cf34634 into apache:main Dec 12, 2022
@Ranxy Ranxy deleted the large_range_reader branch December 12, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration test for read a larger range
2 participants