Implement ListMultipartUploads

quentin commented

2021-12-14 10:53:47 +00:00

Owner

Implement ListMultipartUploads.
This is also a refactor of ListObjects and ListObjectsV2.

It took me some times as I wanted to propose the following things:

Using an iterator instead of the loop+goto pattern. I find it easier to read and it should enable some optimizations. For example, when consuming keys of a common prefix, we do many redundant checks while the only thing to do is to check if the following key is still part of the common prefix.
Try to name things (see ExtractionResult and RangeBegin enums) and to separate concerns (see ListQuery and Accumulator)
An IO closure to make unit tests possibles.
Unit tests, to track regressions and document how to interact with the code
Integration tests with s3api. In the future, I would like to move them in Rust with the aws rust SDK.

Merging of the logic of ListMultipartUploads and ListObjects was not a goal but a consequence of the previous modifications.

Some points that we might want to discuss:

ListObjectsV1, when using pagination and delimiters, has a weird behavior (it lists multiple times the same prefix) with aws s3api due to the fact that it can not use our optimization to skip the whole prefix. It is independant from my refactor and can be tested with the commented s3api tests in test-smoke.sh. It probably has the same weird behavior on the official AWS S3 implementation.
Considering ListMultipartUploads, I had to "abuse" upload id marker to support prefix skipping. I send an upload-id-marker with the hardcoded value include to emulate your "including" token.
Some ways to test ListMultipartUploads with existing software (my tests are limited to s3api for now).

Implement [ListMultipartUploads](https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListMultipartUploads.html). This is also a refactor of ListObjects and ListObjectsV2. It took me some times as I wanted to propose the following things: - Using an iterator instead of the loop+goto pattern. I find it easier to read and it should enable some optimizations. For example, when consuming keys of a common prefix, we do many [redundant checks](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/main/src/api/s3_list.rs#L125-L156) while the only thing to do is to [check if the following key is still part of the common prefix](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/feature/s3-multipart-compat/src/api/s3_list.rs#L476). - Try to name things (see ExtractionResult and RangeBegin enums) and to separate concerns (see ListQuery and Accumulator) - An IO closure to make unit tests possibles. - Unit tests, to track regressions and document how to interact with the code - Integration tests with `s3api`. In the future, I would like to move them in Rust with the aws rust SDK. Merging of the logic of ListMultipartUploads and ListObjects was not a goal but a consequence of the previous modifications. Some points that we might want to discuss: - ListObjectsV1, when using pagination and delimiters, has a weird behavior (it lists multiple times the same prefix) with `aws s3api` due to the fact that it can not use our optimization to skip the whole prefix. It is independant from my refactor and can be tested with the commented `s3api` tests in `test-smoke.sh`. It probably has the same weird behavior on the official AWS S3 implementation. - Considering ListMultipartUploads, I had to "abuse" upload id marker to support prefix skipping. I send an `upload-id-marker` with the hardcoded value `include` to emulate your "including" token. - Some ways to test ListMultipartUploads with existing software (my tests are limited to s3api for now).

quentin added the

scope

s3-api

label 2021-12-15 08:21:36 +00:00

quentin force-pushed feature/s3-multipart-compat from 941ae0e12a to e79cfbd330

2022-01-07 16:33:12 +00:00

Compare

quentin changed title from ~~WIP: feature/s3-multipart-compat~~ to Implement ListMultipartUploads

2022-01-07 16:36:08 +00:00

quentin force-pushed feature/s3-multipart-compat from e79cfbd330 to a5662bc998

2022-01-07 16:38:26 +00:00

Compare

lx commented

2022-01-10 15:32:11 +00:00

Owner

LGTM.

There is one thing I'm worried about, this hack:

fn key_after_prefix(pfx: &str) -> String {
	let mut first_key_after_prefix = pfx.to_string();
	let tail = first_key_after_prefix.pop().unwrap();
	first_key_after_prefix.push(((tail as u8) + 1) as char);
	first_key_after_prefix
}

What if the last character in the key is an utf8 character that cannot be cast into u8? I think either the case will panic, the + will either overflow or the + will throw a panic. In either cases, I think we are doing things wrong.

My proposed solution would be to use Vec<u8> everywhere for cursors instead of strings, and have a key_after_prefix that looks like this:

fn key_after_prefix(pfx: &str) -> Vec<u8> {
	let mut first_key_after_prefix = pfx.as_bytes().to_vec();
	let tail = first_key_after_prefix.pop().unwrap();
	first_key_after_prefix.push(tail + 1);
	first_key_after_prefix
}

Here we are sure that tail is a byte that is not 0xFF (because no unicode character contains a 0xFF in its unicode representation), therefore adding 1 is sure to be a valid u8 value.

To make this work, we need an adapted get_range so that the start position can also be a raw vec<u8> that contains arbitrary bytes for the sort key. (If we look how keys are stored in Sled: Objects have a key field that is a String, but in Sled they are interpreted as raw bytes and stored in the tree in the order of byte strings, and not in the dictionary order of unicode strings. Currently get_range expects a starting position that is the underlying sort key type of the entry type, i.e. String in the case of Object, and it does itself the conversion to underlying bytes to get the start entry in the Sled tree. But we could very simply have a variant that takes the start key as raw bytes instead)

Otherwise, I didn't see anything obviously wrong, and the unit tests seem to suggest that things are fine. Now we have to continue testing the code by using it.

I was wondering if we really need to return <Owner> and <Initiator> for multipart uploads: we don't return the <Owner> in ListObjects and that doesn't seem to be an issue for the moment. And I find it wierd to return fake data that looks plausible: here we are returning the info of the key that is making the query, but that means that different users will see different owners and initiators, which might confuse them if they believe this is true data stored in the bucket. So I'd either remove returning this, or replace it by something that is obviously dummy data (e.g. GKdummykey or something) and that is constant between calls.

A few remarks of style:

ListCommonQuery should be called ListQueryCommon (better English)
I'd rename field pub com: ListCommonQuery into pub common: ListCommonQuery in application of the following non-written rule: only use abbreviated words as identifiers in local variables, never in function names, struct names, or struct fields.
we need a comment that explains what is the Option<String> in ExtractionResult::SkipTo and RangeBegin::IncludingKey ; I figured it's the last key we know in the bucket that is strictly smaller than the first key we want to get, if we know it, but that's not obvious from reading the code
I think the parse function should get a more explicit name, for example: drive_list_query

LGTM. There is one thing I'm worried about, this hack: ```rust fn key_after_prefix(pfx: &str) -> String { let mut first_key_after_prefix = pfx.to_string(); let tail = first_key_after_prefix.pop().unwrap(); first_key_after_prefix.push(((tail as u8) + 1) as char); first_key_after_prefix } ``` What if the last character in the key is an utf8 character that cannot be cast into `u8`? I think either the case will panic, the `+` will either overflow or the `+` will throw a panic. In either cases, I think we are doing things wrong. My proposed solution would be to use `Vec<u8>` everywhere for cursors instead of strings, and have a `key_after_prefix` that looks like this: ```rust fn key_after_prefix(pfx: &str) -> Vec<u8> { let mut first_key_after_prefix = pfx.as_bytes().to_vec(); let tail = first_key_after_prefix.pop().unwrap(); first_key_after_prefix.push(tail + 1); first_key_after_prefix } ``` Here we are sure that tail is a byte that is not `0xFF` (because no unicode character contains a `0xFF` in its unicode representation), therefore adding 1 is sure to be a valid `u8` value. To make this work, we need an adapted `get_range` so that the start position can also be a raw `vec<u8>` that contains arbitrary bytes for the sort key. (If we look how keys are stored in Sled: Objects have a `key` field that is a String, but in Sled they are interpreted as raw bytes and stored in the tree in the order of byte strings, and not in the dictionary order of unicode strings. Currently `get_range` expects a starting position that is the underlying sort key type of the entry type, i.e. String in the case of Object, and it does itself the conversion to underlying bytes to get the start entry in the Sled tree. But we could very simply have a variant that takes the start key as raw bytes instead) Otherwise, I didn't see anything obviously wrong, and the unit tests seem to suggest that things are fine. Now we have to continue testing the code by using it. I was wondering if we really need to return `<Owner>` and `<Initiator>` for multipart uploads: we don't return the `<Owner>` in ListObjects and that doesn't seem to be an issue for the moment. And I find it wierd to return fake data that looks plausible: here we are returning the info of the key that is making the query, but that means that different users will see different owners and initiators, which might confuse them if they believe this is true data stored in the bucket. So I'd either remove returning this, or replace it by something that is obviously dummy data (e.g. `GKdummykey` or something) and that is constant between calls. A few remarks of style: - `ListCommonQuery` should be called `ListQueryCommon` (better English) - I'd rename field `pub com: ListCommonQuery` into `pub common: ListCommonQuery` in application of the following non-written rule: only use abbreviated words as identifiers in local variables, never in function names, struct names, or struct fields. - we need a comment that explains what is the `Option<String>` in `ExtractionResult::SkipTo` and `RangeBegin::IncludingKey` ; I figured it's the last key we know in the bucket that is strictly smaller than the first key we want to get, if we know it, but that's not obvious from reading the code - I think the `parse` function should get a more explicit name, for example: `drive_list_query`

quentin force-pushed feature/s3-multipart-compat from a5662bc998 to cccc59d544

2022-01-12 13:13:33 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from cccc59d544 to 7fe2aacaad

2022-01-12 13:15:08 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from 7fe2aacaad to ccdf6e4369

2022-01-12 14:14:02 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from ccdf6e4369 to 22a7f82de1

2022-01-12 15:25:15 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from 22a7f82de1 to d0de9c7211

2022-01-12 16:10:34 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from d0de9c7211 to c3b6dd9347

2022-01-12 16:13:37 +00:00

Compare

quentin commented

2022-01-12 16:20:08 +00:00

Author

Owner

I have implemented all your suggestions:

you will find a new version of key_after_prefix in c3b6dd9347d6e54ed50feb4180163bbda1c2a06f + some unit tests for this function, including tests for utf8 surrogates and max char.
Owner and Initiator now returns dummy values
ListCommonQuery is now named ListQueryCommon
com is now named common
For ExtractionResult+RangeBegin I added some comments for SkipTo+IncludingKey + I change the tuple enum to a struct enum to better explain the role of each entries.
I renamed parse as fetch_list_entries

Once you have reviewed my changes and the CI is green, you can merge the PR :-)

I have implemented all your suggestions: - you will find a new version of `key_after_prefix` in c3b6dd9347d6e54ed50feb4180163bbda1c2a06f + some unit tests for this function, including tests for utf8 surrogates and max char. - Owner and Initiator now returns dummy values - ListCommonQuery is now named ListQueryCommon - com is now named common - For ExtractionResult+RangeBegin I added some comments for SkipTo+IncludingKey + I change the tuple enum to a struct enum to better explain the role of each entries. - I renamed parse as fetch_list_entries Once you have reviewed my changes and the CI is green, you can merge the PR :-)

quentin changed title from ~~Implement ListMultipartUploads~~ to WIP: Implement ListMultipartUploads

2022-01-12 16:29:16 +00:00

quentin force-pushed feature/s3-multipart-compat from c3b6dd9347 to fd83a2f8ad

2022-01-12 16:38:43 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from fd83a2f8ad to 939f8d3e75

2022-01-12 16:44:17 +00:00

Compare

quentin force-pushed feature/s3-multipart-compat from 939f8d3e75 to 3bbc263027

2022-01-12 16:51:56 +00:00

Compare

quentin changed title from ~~WIP: Implement ListMultipartUploads~~ to Implement ListMultipartUploads

2022-01-12 16:52:57 +00:00

lx merged commit b4592a00fe into main

2022-01-12 18:04:56 +00:00

lx referenced this pull request from a commit

2022-01-12 18:04:56 +00:00

Implement ListMultipartUploads (#171)

quentin referenced this pull request

2022-01-21 11:02:45 +00:00

Docker Registry requires ListMultipartUploads and ListParts #103

Implement ListMultipartUploads #171