Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] #1233

kuenishi · 2015-09-02T03:43:53Z

@angrycub 's awesome branch is for 1.5, this is a rebased version (although I updated some flavor for 2.1). Note that max key length of S3 is 1024 .

This allows for an optimization for single object queries, causing the fold to only retrieve the item of interest.

shino · 2015-09-02T05:52:29Z

Just a note for self, AWS S3 response for too long key:

HTTP/1.1 400 Bad Request
x-amz-request-id: 7A0D868D37BA08C9
x-amz-id-2: 4vRTdtZV6GAms5oAd0W95d08/KkjBjlLPXSus5fIcb7QzpLqlyuUJYqjiCz73WVdi4B5AMhtHVU=
Content-Type: application/xml
Transfer-Encoding: chunked
Date: Wed, 02 Sep 2015 05:51:29 GMT
Connection: close
Server: AmazonS3

133
<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>KeyTooLongError</Code><Message>Your key is too long</Message><Size>1029</Size><MaxSizeAllowed>1024</MaxSizeAllowed><RequestId>7A0D868D37BA08C9</RequestId><HostId>4vRTdtZV6GAms5oAd0W95d08/KkjBjlLPXSus5fIcb7QzpLqlyuUJYqjiCz73WVdi4B5AMhtHVU=</HostId></Error>
0

shino · 2015-09-02T06:14:31Z

I've got two error when prefix is longer than 1024. One is expected (does not mean good):

15:11:09.453 [error] gen_fsm <0.984.0> in state prepare terminated with reason: bad argument in call to binary:copy(<<"ÿ">>, -1) in riak_cs_utils:big_end_key/1 line 447

but another is unexpected:

15:11:09.454 [error] FORMAT ERROR: [67,82,65,83,72,32,82,69,80,79,82,84,32,80,114,111,99,101,115,115,32,"<0.984.0>",32,119,105,116,104,32,"1",32,110,101,105,103,104,98,111,117,114,115,32,"exited",32,119,105,116,104,32,114,101,97,115,111,110,58,32,["bad argument in call to ",["binary",58,"copy",40,["<<","\"ÿ\"",">>"],44,32,"-1",41]," in ",[["riak_cs_utils",58,"big_end_key",47,"1"],[32,108,105,110,101,32,"447"]]]] []

kuenishi · 2015-09-02T06:39:39Z

Looks like I'd better add some guard both there and malformed_request/2 .

shino · 2015-09-02T06:52:03Z

Include max key length check into this PR? If so, There has to be an option max_key_length like max_buckets_per_user for compatibility, isn't it?

kuenishi · 2015-09-02T06:56:15Z

No, it's already defined in this pull request as macro in riak_cs.hrl. Have we supported key length longer than 1024? If yes, I'll update some in this pull request as well as safe guards. At least we need this optimization, don't we?

shino · 2015-09-02T07:29:51Z

I was just caremad because previous versions of riak cs supports keys
which are more than 1024 bytes, probably because such limit was just
not considered. In max_buckets_per_user case, the limit is made
configurable for users who don't care strict AWS S3 compatibility.
Then, whether it should be in this case, max key length. My current
feeling is 1024 is sufficiently long and it has not be configurable.

So, if you are going to add validation to key length, I agree with you.
I'm afraid a little it will be scope creep because almost all WM handlers are affected 🙈

shino · 2015-09-02T07:31:41Z

P.S. For GET Object, AWS S3 responds with 400 KeyTooLongError

kuenishi · 2015-09-02T08:46:51Z

Will add some r_ts tomorrow.

shino · 2015-09-02T08:57:07Z

PR for lager https://github.com/basho/lager/pull/292 (on top of 2.2.0)

shino · 2015-09-02T09:13:08Z

It seems that list objects with prefix with >1024 bytes is still failing with 7c4aa3b

shino · 2015-09-03T02:15:48Z

Memo: AWS S3 GET Bucket responds OK for >1024 bytes prefix

https://gist.github.com/shino/4b5400ace3ba1c8f4279

kuenishi · 2015-09-07T01:52:37Z

I think it's ready again.

shino · 2015-09-07T02:16:39Z

src/riak_cs_utils.erl

+-spec big_end_key(Prefix::binary() | undefined) -> binary().
+big_end_key(undefined) ->
+    big_end_key(<<>>);
+big_end_key(Prefix) when byte_size(Prefix) > ?MAX_S3_KEY_LENGTH ->


Why isn't the value in application env used here?

It's difficult to guess the "right" (not too bad) behavior in this situation...
The save start key and end one results in at most single entry. Then, is it better to use <<Prefix/binary, 255>> here?

Ah, just forgot about app env. Will push an update.

shino · 2015-09-07T02:31:09Z

src/riak_cs_wm_utils.erl

-    LocalCtx = LocalCtx0#key_context{bucket=Bucket, key=Key},
-    Ctx#context{bucket=Bucket,
-                local_context=LocalCtx}.
+    case byte_size(unicode:characters_to_binary(Key)) of


I think Key is just a sequence of bytes instead of sequence of Unicode codepoints.

Counter example: 600 0x81s, less then 1024.

% for i in {1..600}; do echo -n %81; done | read LT1k % S3CURL=.s3curl.15018.alice s3curl.pl --put rebar.config --id cs -- -x 127.0.0.1:15018 -s http://test.s3.amazonaws.com/${LT1k} <?xml version="1.0" encoding="UTF-8"?> <Error><Code>KeyTooLongError</Code><Message>Your key is too long</Message> <Size>1200</Size><MaxSizeAllowed>1024</MaxSizeAllowed><RequestId></RequestId></Error>

I thought I've fixed it 😢

kuenishi · 2015-09-07T03:29:29Z

updated

shino · 2015-09-07T03:47:23Z

src/riak_cs_utils.erl

+                  MaxLen when byte_size(Prefix) > MaxLen ->
+                      <<>>;
+                  MaxLen ->
+                      binary:copy(<<255>>, MaxLen - byte_size(Prefix))


This fails if MaxLen is unlimited

shino · 2015-09-07T06:12:03Z

riak_test/src/list_objects_test_helper.erl

@@ -105,6 +105,13 @@ test(UserConfig) ->
    ?assert(lists:member([{prefix, "0/"}], CommonPrefixes3)),
    verify_object_list(ObjList4, 30),

+    %% Don't fail even if Prefix is longer than 1024, even if keys are
+    %% restricted to be shorter than it. That's S3.


shino · 2015-09-07T07:51:15Z

Cool optimization! Great collaboration work @angrycub & @kuenishi 👍

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Reviewed-by: shino

kuenishi · 2015-09-07T08:44:39Z

@borshop merge

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Reviewed-by: shino

Basho-JIRA · 2015-09-07T08:51:12Z

Release note: GET Bucket API (aka listing objects in a bucket) with prefix specified had a room for optimization, that had been specifying too loose end key for folding objects in Riak. With this change more tight end key is being specified as end key for folding objects in Riak to omit unnecessary fold in vnodes. Limitation to max length of keys has been also introduced, which can be specified as 1024 by default. This means keys no longer than 1024 bytes could be PUT, GET nor DELETED any more unless max_key_length explicitly specified as more than 1024 in riak-cs.conf. Operators who want to preserve old behaviour about key length may also use unlimited for max_key_length.

_[posted via JIRA by Kota Uenishi]_

kuenishi · 2015-09-07T08:53:29Z

@borshop: retry

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Reviewed-by: shino Conflicts: src/riak_cs_s3_response.erl src/riak_cs_wm_object.erl src/riak_cs_wm_object_upload_part.erl

angrycub added 3 commits September 2, 2015 12:37

Added the prefix to the start of EndKey

dd0f554

This allows for an optimization for single object queries, causing the fold to only retrieve the item of interest.

Corrected continuations

dcb15d7

Added trailing whitespace

609f8d8

kuenishi added this to the 2.1.0 milestone Sep 2, 2015

Basho-JIRA changed the title ~~Merge CV's list-objects optimization~~ Merge CV's list-objects optimization [JIRA: RCS-275] Sep 2, 2015

Basho-JIRA added the JIRA: To Do label Sep 2, 2015

shino changed the title ~~Merge CV's list-objects optimization [JIRA: RCS-275]~~ Merge CV's list-objects prefix optimization [JIRA: RCS-275] Sep 2, 2015

Add limit to max key length

7c4aa3b

kuenishi changed the title ~~Merge CV's list-objects prefix optimization [JIRA: RCS-275]~~ Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Sep 2, 2015

Basho-JIRA added JIRA: In Progress JIRA: Needs Review and removed JIRA: To Do JIRA: In Progress labels Sep 7, 2015

Basho-JIRA assigned kuenishi Sep 7, 2015

Add tests and fix prefix in list objects

7f6a304

shino reviewed Sep 7, 2015
View reviewed changes

Handle too long prefix as to preserve at most single result

f8796c3

shino reviewed Sep 7, 2015
View reviewed changes

kuenishi added 2 commits September 7, 2015 11:53

Use raw byte length of Unicode

5bf1f0e

length/0 rather than byte_size/0

abf8040

shino reviewed Sep 7, 2015
View reviewed changes

Add unlimited case branch

8642741

shino reviewed Sep 7, 2015
View reviewed changes

borshop added a commit that referenced this pull request Sep 7, 2015

Merge pull request #1233 from basho/feature/optimize-list-objects

ba18a99

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Reviewed-by: shino

borshop added a commit that referenced this pull request Sep 7, 2015

Merge pull request #1233 from basho/feature/optimize-list-objects

39b7075

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] Reviewed-by: shino

borshop merged commit 8642741 into develop Sep 7, 2015

kuenishi deleted the feature/optimize-list-objects branch September 7, 2015 09:09

Basho-JIRA added JIRA: Closed and removed JIRA: Needs Review labels Sep 7, 2015

kuenishi mentioned this pull request Sep 18, 2015

Backport #1233 to 2.0 #1253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] #1233

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] #1233

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 3, 2015

kuenishi commented Sep 7, 2015

shino Sep 7, 2015

shino Sep 7, 2015

kuenishi Sep 7, 2015

shino Sep 7, 2015

shino Sep 7, 2015

kuenishi Sep 7, 2015

kuenishi commented Sep 7, 2015

shino Sep 7, 2015

shino Sep 7, 2015

shino commented Sep 7, 2015

kuenishi commented Sep 7, 2015

Basho-JIRA commented Sep 7, 2015

kuenishi commented Sep 7, 2015

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] #1233

Merge CV's list-objects prefix optimization + Add key length limit [JIRA: RCS-275] #1233

Conversation

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

kuenishi commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 2, 2015

shino commented Sep 3, 2015

kuenishi commented Sep 7, 2015

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

kuenishi commented Sep 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shino commented Sep 7, 2015

kuenishi commented Sep 7, 2015

Basho-JIRA commented Sep 7, 2015

kuenishi commented Sep 7, 2015