Enable bucket cache in v2 s3 proxy #17022

007DXR · 2023-03-07T03:37:44Z

What changes are proposed in this pull request?

Create 'BUCKET_CACHE' in v2 s3 proxy to reduce time cost of checking bucket path in the same way as PR #16806.

Why are the changes needed?

To keep the consistence between v2 s3 proxy and v1 s3 proxy.
To speed up the Alluxio proxy efficency to deal with requests .

Does this PR introduce any user facing changes?

If enabling the cache, alluxio will cache bucket path statistics for specified time period(configured in alluxio-site.properties file). Be careful to use this cache because Alluxio S3 API will behave differently from AWS S3 API when coming an illegal request. This bucket path cache is swithed off by default.

007DXR · 2023-03-07T04:44:32Z

@Jackson-Wang-7 @lucyge2022 could you please take a look?

add `overwrite` and `checkpath` tags to `copyFilePOptions` in `CopyObjectTask()`

007DXR · 2023-03-12T11:20:14Z

@lucyge2022 This PR introduces v2 s3 proxy which relates to yours. Could you review it?

lucyge2022 · 2023-03-13T20:31:48Z

core/server/proxy/src/main/java/alluxio/proxy/s3/S3Handler.java

+      .expireAfterWrite(
+          Configuration.global().getMs(PropertyKey.PROXY_S3_BUCKETPATHCACHE_TIMEOUT_MS),
+          TimeUnit.MILLISECONDS)
+      .build();


why keep boolean as value? if u delete the bucket why can't just remove it?

also why keep alluxiouri as key? in S3RestUtils.checkPathIsAlluxioDirectory we anyways are using bucketPath to check right?

for the first question, BUCKET_PATH_CACHE is a cache and there is a nearly perfect implement of cache in Alluxio while this cache is map type. So I set BUCKET_PATH_CACHE as a map type as well. boolean is most suitable as the second key of this map.

for the second question, S3RestUtils.checkPathIsAlluxioDirectory calls master with arg AlluxioURI(bucketPath) to check. so AlluxioURI(bucketPath) is better than bucketPath to be the first key of BUCKET_PATH_CACHE.

for the first question, BUCKET_PATH_CACHE is a cache and there is a nearly perfect implement of cache in Alluxio while this cache is map type. So I set BUCKET_PATH_CACHE as a map type as well. boolean is most suitable as the second key of this map.

isn't this guava cache instead of alluxio impl?

for the second question, S3RestUtils.checkPathIsAlluxioDirectory calls master with arg AlluxioURI(bucketPath) to check. so AlluxioURI(bucketPath) is better than bucketPath to be the first key of BUCKET_PATH_CACHE.

but in every call the use of S3RestUtils.checkPathIsAlluxioDirectory also uses bucketPath, so now its 1)conversion of bucketpath to alluxiouri 2) check cache with it 3) if not in cache checkPathIsAlluxioDirectory with bucketpath again and another conversion to alluxiouri and call master? u see my point? since from s3 api layer its already using string type bucketpath, why not cache the bucketpath str only?

isn't this guava cache instead of alluxio impl?

It's guava cache.

also why keep alluxiouri as key? in S3RestUtils.checkPathIsAlluxioDirectory we anyways are using bucketPath to check right?

I changed the first key of bucketPathCache to str type. Is the change right?

if u delete the bucket why can't just remove it?

if we introduce negative cache in the future, setting false will work different from removing it.

also why keep alluxiouri as key? in S3RestUtils.checkPathIsAlluxioDirectory we anyways are using bucketPath to check right?

I changed the first key of bucketPathCache to str type. Is the change right?

yeah str is better to avoid multiple str<->alluxiouri conversion, just want to make sure that u r aware of the choices : )

if u delete the bucket why can't just remove it?

if we introduce negative cache in the future, setting false will work different from removing it.

Actually here in current way of using cache in checkPathIsAlluxioDirectory, the cache is not treated as negative cache, AKA its not acting back directly from the cache if 1)cache hit and 2) value is negative, it still went ahead to check master instead of throwing bucket-not-exist exception. And regarding proxy should stay stateless at all times, in order to prevent stale entires (e.g. delete bucket from this proxy but create same bucket on another proxy) , we should never use a negative cache here. So I would suggest : 1) keep the current way of check on master on cache miss 2) remove the boolean if u can find a way with guava cache or internal impl with a expiration, 3) or keep the boolean still but give it a comment of this boolean is simply a placeholder Does that make sense to u?

I keep the boolean still but give it a comment of the usage of boolean

lucyge2022 · 2023-03-13T20:54:20Z

core/server/proxy/src/main/java/alluxio/proxy/s3/S3ObjectTask.java

@@ -733,6 +739,7 @@ public Response continueTask() {
                  .setWriteType(S3RestUtils.getS3WriteType())
                  .putAllXattr(xattrMap).setXattrPropStrat(XAttrPropagationStrategy.LEAF_NODE)
                  .setOverwrite(true)
+                  .setCheckS3BucketPath(true)


why is this check needed if we have the checkPathIsAlluxioDirectory step? it will anyway prevent any s3 create obj to create bucket folder in the inode tree is it?

master didn't throw exception when the bucket doesn't exist without setCheckS3BucketPath(true)

but with checkPathIsAlluxioDirectory in front of all createobj involved call no one can create a file without creating the bucket folder first isnt it?

e.g. someone create bucket on this proxy , delete same bucket from another proxy. if we create obj on this proxy after that,checkPathIsAlluxioDirectory tells the bucket exists . but it's wrong. so setCheckS3BucketPath(true) is necessary.

@lucyge2022 can this PR be merged?

understood your point, actually in this case I have another question, in this case, would other calls gets a incorrect error response, say I delete the bucket from another proxy while this proxy hasn't expire the bucket path, if I issue a call like putobjecttagging to this proxy, would it become a internal error or NoSuchKey bcos of filenotexistexception from master instead of NoSuchBucket error?

yes. in this case proxy throws NoSuchKey exception. We can consider NoSuchBucket shows more details than NoSuchKey. but NoSuchKey is not wrong.

007DXR · 2023-04-14T02:43:32Z

@lucyge2022 I disabled the bucket path cache by default and added some description to PropertyKey.java file. Is it OK?

lucyge2022 · 2023-04-14T04:37:13Z

core/common/src/main/java/alluxio/conf/PropertyKey.java

-              + "Set 0min to disable the cache.")
+              + "Set 0min to disable the cache. If enabling the cache, "
+              + "be careful that Alluxio S3 API will behave differently from AWS S3 API"
+              + " when coming an illegal request.")


this could be "be careful that Alluxio S3 API will behave differently from AWS S3 API if bucketpath cache entries becomes stale"

lucyge2022 · 2023-04-14T04:41:33Z

some minor comment, otherwise LGTM. thanks

007DXR · 2023-04-18T02:42:35Z

@Jackson-Wang-7

jiacheliu3 · 2023-04-18T02:46:19Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? Create 'BUCKET_CACHE' in v2 s3 proxy to reduce time cost of checking bucket path in the same way as PR Alluxio#16806. ### Why are the changes needed? To keep the consistence between v2 s3 proxy and v1 s3 proxy. To speed up the Alluxio proxy efficency to deal with requests . ### Does this PR introduce any user facing changes? If enabling the cache, alluxio will cache bucket path statistics for specified time period(configured in alluxio-site.properties file). Be careful to use this cache because Alluxio S3 API will behave differently from AWS S3 API when coming an illegal request. This bucket path cache is swithed off by default. pr-link: Alluxio#17022 change-id: cid-a6ae6484aab704bc39ce3a02d22d70bd633cc6a6

modify checkPathIsAlluxioDirectory

28d4747

alluxio-bot assigned tcrain Mar 7, 2023

modify checkPathIsAlluxioDirectory

c454ee9

remove checkBucketPath tag of filePOptions in UploadPartTask().

0563404

add `overwrite` and `checkpath` tags to `copyFilePOptions` in `CopyObjectTask()`

007DXR mentioned this pull request Mar 10, 2023

Enable v2 s3 proxy by default #16937

Merged

lucyge2022 reviewed Mar 13, 2023

View reviewed changes

007DXR added 5 commits March 16, 2023 14:05

change the first key of bucketPathCache to str type

ee7f989

cache the bucketpath as str

48cfc0a

Update S3BucketTask.java

2101665

Update S3BucketTask.java

a02e742

Update S3RestUtils.java

4bde244

007DXR closed this Mar 16, 2023

007DXR added 2 commits March 16, 2023 17:04

Update S3RestServiceHandler.java

ae063c5

Update S3RestUtils.java

8953fe8

007DXR reopened this Mar 16, 2023

007DXR and others added 5 commits March 17, 2023 10:36

add comments about BUCKET_PATH_CACHE

677eca2

add comments about BUCKET_PATH_CACHE

b7b988f

Merge branch 'Alluxio:master' into v2_s3_proxy

e4046ec

disable the bucket path cache by default.

e5c7cdd

Merge remote-tracking branch 'origin/v2_s3_proxy' into v2_s3_proxy

ab3d25c

alluxio-bot added the API Change Changes covering public API label Apr 13, 2023

add some description to configuration file.

1615bbc

lucyge2022 reviewed Apr 14, 2023

View reviewed changes

add some description to configuration file.

e904b92

jiacheliu3 added the type-feature This issue is a feature request label Apr 18, 2023

alluxio-bot merged commit 46270b8 into Alluxio:master Apr 18, 2023
19 checks passed

Xenorith added the area-s3-rest-api label Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable bucket cache in v2 s3 proxy #17022

Enable bucket cache in v2 s3 proxy #17022

007DXR commented Mar 7, 2023 •

edited

007DXR commented Mar 7, 2023

007DXR commented Mar 12, 2023 •

edited

lucyge2022 Mar 13, 2023

lucyge2022 Mar 13, 2023

007DXR Mar 14, 2023 •

edited

007DXR Mar 14, 2023

lucyge2022 Mar 14, 2023

007DXR Mar 16, 2023 •

edited

007DXR Mar 16, 2023

lucyge2022 Mar 16, 2023

lucyge2022 Mar 16, 2023

007DXR Mar 21, 2023

lucyge2022 Mar 13, 2023

007DXR Mar 14, 2023

lucyge2022 Mar 16, 2023

007DXR Mar 17, 2023

007DXR Mar 21, 2023

lucyge2022 Mar 21, 2023 •

edited

007DXR Mar 22, 2023

007DXR commented Apr 14, 2023

lucyge2022 Apr 14, 2023

lucyge2022 commented Apr 14, 2023

007DXR commented Apr 18, 2023

jiacheliu3 commented Apr 18, 2023

Enable bucket cache in v2 s3 proxy #17022

Enable bucket cache in v2 s3 proxy #17022

Conversation

007DXR commented Mar 7, 2023 • edited

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

007DXR commented Mar 7, 2023

007DXR commented Mar 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

007DXR Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

007DXR Mar 16, 2023 • edited

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

Choose a reason for hiding this comment

lucyge2022 Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

007DXR commented Apr 14, 2023

Choose a reason for hiding this comment

lucyge2022 commented Apr 14, 2023

007DXR commented Apr 18, 2023

jiacheliu3 commented Apr 18, 2023

007DXR commented Mar 7, 2023 •

edited

007DXR commented Mar 12, 2023 •

edited

007DXR Mar 14, 2023 •

edited

007DXR Mar 16, 2023 •

edited

lucyge2022 Mar 21, 2023 •

edited