feat(core): implement if_modified_since and if_unmodified_since for read_with and reader_with#5500
Conversation
github-actions
bot
added
the
releases-note/feat
|
Tests failed in |
|
minio should support those headers: minio/minio#1098 |
|
meteorgan
force-pushed
the
conditional_reader
branch
from
cbe9efb to
ad7eac7
Compare
Ooops, it does have the requirement for |
core/tests/behavior/async_read.rs
Outdated
| let bs = reader.read(..).await?.to_bytes(); | ||
| assert_eq!(bs, content); | ||
|
|
||
| sleep(Duration::from_secs(3)).await; |
There was a problem hiding this comment.
It's better to remove this sleep.
core/tests/behavior/async_read.rs
Outdated
|
|
||
| sleep(Duration::from_secs(3)).await; | ||
|
|
||
| let one_second_ago_check = chrono::Utc::now() - chrono::Duration::seconds(1); |
There was a problem hiding this comment.
How about using chrono::Utc::now() + chrono::Duration::seconds(10)? Also, the name one_second_ago_check is incorrect. I recommend avoiding lengthy names; a simple since should suffice.
There was a problem hiding this comment.
It doesn't work for S3. It might be considered an invalid time by S3 . (but I haven't found any official resources about this yet)
There was a problem hiding this comment.
Maybe We can just use chrono::Utc::now() here. but I'm not sure if this test will remain stable.
There was a problem hiding this comment.
By the way, Using a future time works for both Minio and Ceph.
There was a problem hiding this comment.
It doesn't make sense for the server to reject a future time. I believe AWS S3 is likely to support it, so we can give it a try.
There was a problem hiding this comment.
I've tested it in my local environment, and the result is the same as using the AWS CLI. When using a future time in if-modified-since, S3 always return a 200 status.
There was a problem hiding this comment.
Interesting. Ok, let's test in this way:
- Create a file.
- Retrieve its last modified time.
- Read it using
last_modified - 1s(this should work). - Wait for 1 second.
- Attempt to read it using
last_modified + 1s(this should result in an error).
What do you think?
There was a problem hiding this comment.
Ok, At least, this approach can help reduce the waiting time.
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Most changes look to me now, the only thing left here is the tests.
meteorgan
force-pushed
the
conditional_reader
branch
from
100e860 to
3176e2d
Compare
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you @meteorgan for working on this!
2 participants
Which issue does this PR close?
Part of #5486.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
We'll add two methods
if_modified_sinceandif_unmodified_sinceforread_withandreader_withAPI