Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
github-actions[bot] closed pull request #10045: HDDS-13532. Refactor BucketEndpoint.get() URL: https://github.com/apache/ozone/pull/10045 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
github-actions[bot] commented on PR #10045: URL: https://github.com/apache/ozone/pull/10045#issuecomment-4910230287 Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
github-actions[bot] commented on PR #10045: URL: https://github.com/apache/ozone/pull/10045#issuecomment-4644580224 This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
sreejasahithi commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3254096450
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
Review Comment:
Can you please add a class level javadoc here.
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class BucketListing {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(BucketListing.class);
+
+ private final S3RequestContext context;
+ private final BucketEndpoint endpoint;
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final String continueToken;
+ private String prefix;
+ private String startAfter;
+ private int maxKeys;
+ private OzoneBucket bucket;
+ private Iterator ozoneKeyIterator;
+ private ContinueToken decodedToken;
+
+ private BucketListing(S3RequestContext context, BucketEndpoint endpoint,
String bucketName) {
+this.context = context;
+this.endpoint = endpoint;
+this.bucketName = bucketName;
+this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);
Review Comment:
Can we use OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT here instead?
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,225 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3223286103
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static
org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class BucketListing {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(BucketListing.class);
+
+ private final S3RequestContext context;
+ private final BucketEndpoint endpoint;
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final String continueToken;
+ private String prefix;
+ private String startAfter;
+ private int maxKeys;
+ private String prevKey;
+ private boolean shallow;
+ private OzoneBucket bucket;
+ private Iterator ozoneKeyIterator;
+ private ContinueToken decodedToken;
+
+ private BucketListing(S3RequestContext context, BucketEndpoint endpoint,
String bucketName) {
+this.context = context;
+this.endpoint = endpoint;
+this.bucketName = bucketName;
+this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);
+this.prefix = endpoint.queryParams().get(QueryParams.PREFIX);
+this.continueToken =
endpoint.queryParams().get(QueryParams.CONTINUATION_TOKEN);
+this.startAfter = endpoint.queryParams().get(QueryParams.START_AFTER);
+ }
+
+ static BucketListing fromQueryParams(S3RequestContext context,
BucketEndpoint endpoint,
+ String bucketName) throws IOException, OS3Exception {
+BucketListing listing = new BucketListing(context, endpoint, bucketName);
+listing.validateAndPrepare();
+return listing;
+ }
+
+ Response buildResponse() {
+ListObjectResponse response = initializeListObjectResponse();
+processKeyListing(response);
+return buildFinalResponse(response);
+ }
+
+ int getMaxKeys() {
+return maxKeys;
+ }
+
+ String getPrefix() {
+return prefix;
+ }
+
+ private void validateAndPrepare() throws OS3Exception, IOException {
+validateEncodingType();
+maxKeys = validateMaxKeys(maxKeys);
+
+if (prefix == null) {
+ prefix = "";
+}
+
+if (startAfter == null && marker != null) {
+ startAfter = marker;
+}
+
+decodedToken = ContinueToken.decodeFromString(continueToken);
+prevKey = continueToken != null ? decodedToken.getLastKey() : startAfter;
+shallow = endpoint.isListKeysShallowEnabled()
+&& OZONE_URI_DELIMITER.equals(delimiter);
+
+bucket = context.getVolume().getBucket(bucketName);
+S3Owner.verifyBucketOwnerCondition(endpoint.getHeaders(), bucketName,
bucket.getOwner());
+try {
+ ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
+} catch (OMException ex) {
+ if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
+LOG.debug("Key not
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3223281170
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static
org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class BucketListing {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(BucketListing.class);
+
+ private final S3RequestContext context;
+ private final BucketEndpoint endpoint;
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final String continueToken;
+ private String prefix;
+ private String startAfter;
+ private int maxKeys;
+ private String prevKey;
+ private boolean shallow;
+ private OzoneBucket bucket;
+ private Iterator ozoneKeyIterator;
+ private ContinueToken decodedToken;
+
+ private BucketListing(S3RequestContext context, BucketEndpoint endpoint,
String bucketName) {
+this.context = context;
+this.endpoint = endpoint;
+this.bucketName = bucketName;
+this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);
+this.prefix = endpoint.queryParams().get(QueryParams.PREFIX);
+this.continueToken =
endpoint.queryParams().get(QueryParams.CONTINUATION_TOKEN);
+this.startAfter = endpoint.queryParams().get(QueryParams.START_AFTER);
+ }
+
+ static BucketListing fromQueryParams(S3RequestContext context,
BucketEndpoint endpoint,
+ String bucketName) throws IOException, OS3Exception {
+BucketListing listing = new BucketListing(context, endpoint, bucketName);
+listing.validateAndPrepare();
+return listing;
+ }
+
+ Response buildResponse() {
+ListObjectResponse response = initializeListObjectResponse();
+processKeyListing(response);
+return buildFinalResponse(response);
+ }
+
+ int getMaxKeys() {
+return maxKeys;
+ }
+
+ String getPrefix() {
+return prefix;
+ }
+
+ private void validateAndPrepare() throws OS3Exception, IOException {
+validateEncodingType();
+maxKeys = validateMaxKeys(maxKeys);
+
+if (prefix == null) {
+ prefix = "";
+}
+
+if (startAfter == null && marker != null) {
+ startAfter = marker;
+}
+
+decodedToken = ContinueToken.decodeFromString(continueToken);
+prevKey = continueToken != null ? decodedToken.getLastKey() : startAfter;
+shallow = endpoint.isListKeysShallowEnabled()
+&& OZONE_URI_DELIMITER.equals(delimiter);
+
+bucket = context.getVolume().getBucket(bucketName);
+S3Owner.verifyBucketOwnerCondition(endpoint.getHeaders(), bucketName,
bucket.getOwner());
+try {
+ ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
Review Comment:
I think `ozoneKeyIterator ` is stored and consumed in `processKeyListing()`.
also, `listKeys()` re
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3223278099
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -103,161 +97,18 @@ public Response get(
}
@Override
- Response handleGetRequest(S3RequestContext context, String bucketName)
throws IOException, OS3Exception {
-final String continueToken =
queryParams().get(QueryParams.CONTINUATION_TOKEN);
-final String delimiter = queryParams().get(QueryParams.DELIMITER);
-final String encodingType = queryParams().get(QueryParams.ENCODING_TYPE);
-final String marker = queryParams().get(QueryParams.MARKER);
-int maxKeys = queryParams().getInt(QueryParams.MAX_KEYS, 1000);
-String prefix = queryParams().get(QueryParams.PREFIX, "");
-String startAfter = queryParams().get(QueryParams.START_AFTER);
-
-Iterator ozoneKeyIterator = null;
-ContinueToken decodedToken = ContinueToken.decodeFromString(continueToken);
-OzoneBucket bucket = null;
-
+ Response handleGetRequest(S3RequestContext context, String bucketName)
+ throws IOException, OS3Exception {
try {
- maxKeys = validateMaxKeys(maxKeys);
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
- }
-
- // If continuation token and start after both are provided, then we
- // ignore start After
- String prevKey = continueToken != null ? decodedToken.getLastKey()
- : startAfter;
-
- // If shallow is true, only list immediate children
- // delimited by OZONE_URI_DELIMITER
- boolean shallow = listKeysShallowEnabled
- && OZONE_URI_DELIMITER.equals(delimiter);
-
- bucket = context.getVolume().getBucket(bucketName);
- S3Owner.verifyBucketOwnerCondition(getHeaders(), bucketName,
bucket.getOwner());
-
- ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
-
+ return BucketListing.fromQueryParams(context, this, bucketName)
+ .buildResponse();
} catch (OMException ex) {
getMetrics().updateGetBucketFailureStats(context.getStartNanos());
- if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
-// File not found, continue and send normal response with 0 keyCount
-LOG.debug("Key Not found prefix: {}", prefix);
- } else {
-throw ex;
- }
+ throw ex;
Review Comment:
thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
yandrey321 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3221550031
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static
org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class BucketListing {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(BucketListing.class);
+
+ private final S3RequestContext context;
+ private final BucketEndpoint endpoint;
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final String continueToken;
+ private String prefix;
+ private String startAfter;
+ private int maxKeys;
+ private String prevKey;
+ private boolean shallow;
+ private OzoneBucket bucket;
+ private Iterator ozoneKeyIterator;
+ private ContinueToken decodedToken;
+
+ private BucketListing(S3RequestContext context, BucketEndpoint endpoint,
String bucketName) {
+this.context = context;
+this.endpoint = endpoint;
+this.bucketName = bucketName;
+this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);
+this.prefix = endpoint.queryParams().get(QueryParams.PREFIX);
+this.continueToken =
endpoint.queryParams().get(QueryParams.CONTINUATION_TOKEN);
+this.startAfter = endpoint.queryParams().get(QueryParams.START_AFTER);
+ }
+
+ static BucketListing fromQueryParams(S3RequestContext context,
BucketEndpoint endpoint,
+ String bucketName) throws IOException, OS3Exception {
+BucketListing listing = new BucketListing(context, endpoint, bucketName);
+listing.validateAndPrepare();
+return listing;
+ }
+
+ Response buildResponse() {
+ListObjectResponse response = initializeListObjectResponse();
+processKeyListing(response);
+return buildFinalResponse(response);
+ }
+
+ int getMaxKeys() {
+return maxKeys;
+ }
+
+ String getPrefix() {
+return prefix;
+ }
+
+ private void validateAndPrepare() throws OS3Exception, IOException {
+validateEncodingType();
+maxKeys = validateMaxKeys(maxKeys);
+
+if (prefix == null) {
+ prefix = "";
+}
+
+if (startAfter == null && marker != null) {
+ startAfter = marker;
+}
+
+decodedToken = ContinueToken.decodeFromString(continueToken);
+prevKey = continueToken != null ? decodedToken.getLastKey() : startAfter;
+shallow = endpoint.isListKeysShallowEnabled()
+&& OZONE_URI_DELIMITER.equals(delimiter);
+
+bucket = context.getVolume().getBucket(bucketName);
+S3Owner.verifyBucketOwnerCondition(endpoint.getHeaders(), bucketName,
bucket.getOwner());
+try {
+ ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
+} catch (OMException ex) {
+ if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
+LOG.debug("Key no
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
yandrey321 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3221543738
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static
org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class BucketListing {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(BucketListing.class);
+
+ private final S3RequestContext context;
+ private final BucketEndpoint endpoint;
+ private final String bucketName;
+ private final String delimiter;
+ private final String encodingType;
+ private final String marker;
+ private final String continueToken;
+ private String prefix;
+ private String startAfter;
+ private int maxKeys;
+ private String prevKey;
+ private boolean shallow;
+ private OzoneBucket bucket;
+ private Iterator ozoneKeyIterator;
+ private ContinueToken decodedToken;
+
+ private BucketListing(S3RequestContext context, BucketEndpoint endpoint,
String bucketName) {
+this.context = context;
+this.endpoint = endpoint;
+this.bucketName = bucketName;
+this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);
+this.prefix = endpoint.queryParams().get(QueryParams.PREFIX);
+this.continueToken =
endpoint.queryParams().get(QueryParams.CONTINUATION_TOKEN);
+this.startAfter = endpoint.queryParams().get(QueryParams.START_AFTER);
+ }
+
+ static BucketListing fromQueryParams(S3RequestContext context,
BucketEndpoint endpoint,
+ String bucketName) throws IOException, OS3Exception {
+BucketListing listing = new BucketListing(context, endpoint, bucketName);
+listing.validateAndPrepare();
+return listing;
+ }
+
+ Response buildResponse() {
+ListObjectResponse response = initializeListObjectResponse();
+processKeyListing(response);
+return buildFinalResponse(response);
+ }
+
+ int getMaxKeys() {
+return maxKeys;
+ }
+
+ String getPrefix() {
+return prefix;
+ }
+
+ private void validateAndPrepare() throws OS3Exception, IOException {
+validateEncodingType();
+maxKeys = validateMaxKeys(maxKeys);
+
+if (prefix == null) {
+ prefix = "";
+}
+
+if (startAfter == null && marker != null) {
+ startAfter = marker;
+}
+
+decodedToken = ContinueToken.decodeFromString(continueToken);
+prevKey = continueToken != null ? decodedToken.getLastKey() : startAfter;
+shallow = endpoint.isListKeysShallowEnabled()
+&& OZONE_URI_DELIMITER.equals(delimiter);
+
+bucket = context.getVolume().getBucket(bucketName);
+S3Owner.verifyBucketOwnerCondition(endpoint.getHeaders(), bucketName,
bucket.getOwner());
+try {
+ ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
Review Comment:
do we really need to call listKeys() in validateAndPrepare(), it seems like
an expensive call wh
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
adoroszlai commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3221175873
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -103,161 +97,18 @@ public Response get(
}
@Override
- Response handleGetRequest(S3RequestContext context, String bucketName)
throws IOException, OS3Exception {
-final String continueToken =
queryParams().get(QueryParams.CONTINUATION_TOKEN);
-final String delimiter = queryParams().get(QueryParams.DELIMITER);
-final String encodingType = queryParams().get(QueryParams.ENCODING_TYPE);
-final String marker = queryParams().get(QueryParams.MARKER);
-int maxKeys = queryParams().getInt(QueryParams.MAX_KEYS, 1000);
-String prefix = queryParams().get(QueryParams.PREFIX, "");
-String startAfter = queryParams().get(QueryParams.START_AFTER);
-
-Iterator ozoneKeyIterator = null;
-ContinueToken decodedToken = ContinueToken.decodeFromString(continueToken);
-OzoneBucket bucket = null;
-
+ Response handleGetRequest(S3RequestContext context, String bucketName)
+ throws IOException, OS3Exception {
try {
- maxKeys = validateMaxKeys(maxKeys);
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
- }
-
- // If continuation token and start after both are provided, then we
- // ignore start After
- String prevKey = continueToken != null ? decodedToken.getLastKey()
- : startAfter;
-
- // If shallow is true, only list immediate children
- // delimited by OZONE_URI_DELIMITER
- boolean shallow = listKeysShallowEnabled
- && OZONE_URI_DELIMITER.equals(delimiter);
-
- bucket = context.getVolume().getBucket(bucketName);
- S3Owner.verifyBucketOwnerCondition(getHeaders(), bucketName,
bucket.getOwner());
-
- ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
-
+ return BucketListing.fromQueryParams(context, this, bucketName)
+ .buildResponse();
} catch (OMException ex) {
getMetrics().updateGetBucketFailureStats(context.getStartNanos());
- if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
-// File not found, continue and send normal response with 0 keyCount
-LOG.debug("Key Not found prefix: {}", prefix);
- } else {
-throw ex;
- }
+ throw ex;
Review Comment:
Since `FILE_NOT_FOUND` is handled within `BucketListing`, we don't need
separate `catch (OMException)` anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3219733591
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -103,161 +98,23 @@ public Response get(
}
@Override
- Response handleGetRequest(S3RequestContext context, String bucketName)
throws IOException, OS3Exception {
-final String continueToken =
queryParams().get(QueryParams.CONTINUATION_TOKEN);
-final String delimiter = queryParams().get(QueryParams.DELIMITER);
-final String encodingType = queryParams().get(QueryParams.ENCODING_TYPE);
-final String marker = queryParams().get(QueryParams.MARKER);
-int maxKeys = queryParams().getInt(QueryParams.MAX_KEYS, 1000);
-String prefix = queryParams().get(QueryParams.PREFIX, "");
-String startAfter = queryParams().get(QueryParams.START_AFTER);
-
-Iterator ozoneKeyIterator = null;
-ContinueToken decodedToken = ContinueToken.decodeFromString(continueToken);
-OzoneBucket bucket = null;
-
+ Response handleGetRequest(S3RequestContext context, String bucketName)
+ throws IOException, OS3Exception {
+long startNanos = context.getStartNanos();
+PerformanceStringBuilder perf = context.getPerf();
try {
- maxKeys = validateMaxKeys(maxKeys);
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
- }
-
- // If continuation token and start after both are provided, then we
- // ignore start After
- String prevKey = continueToken != null ? decodedToken.getLastKey()
- : startAfter;
-
- // If shallow is true, only list immediate children
- // delimited by OZONE_URI_DELIMITER
- boolean shallow = listKeysShallowEnabled
- && OZONE_URI_DELIMITER.equals(delimiter);
-
- bucket = context.getVolume().getBucket(bucketName);
- S3Owner.verifyBucketOwnerCondition(getHeaders(), bucketName,
bucket.getOwner());
-
- ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
-
+ return BucketListing.fromQueryParams(this, bucketName)
+ .buildResponse(startNanos, perf);
Review Comment:
got it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3219731507
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -321,11 +173,6 @@ public Response delete(@PathParam(BUCKET) String
bucketName)
}
}
- @Override
- Response handleDeleteRequest(S3RequestContext context, String bucketName) {
-throw newError(S3ErrorTable.NOT_IMPLEMENTED, "DELETE bucket");
- }
-
Review Comment:
ok sorry about that
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -417,7 +264,6 @@ protected void init() {
OZONE_S3G_LIST_MAX_KEYS_LIMIT,
OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT);
-// initialize handlers
Review Comment:
ok sorry about that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
adoroszlai commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3217966562
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -103,161 +98,23 @@ public Response get(
}
@Override
- Response handleGetRequest(S3RequestContext context, String bucketName)
throws IOException, OS3Exception {
-final String continueToken =
queryParams().get(QueryParams.CONTINUATION_TOKEN);
-final String delimiter = queryParams().get(QueryParams.DELIMITER);
-final String encodingType = queryParams().get(QueryParams.ENCODING_TYPE);
-final String marker = queryParams().get(QueryParams.MARKER);
-int maxKeys = queryParams().getInt(QueryParams.MAX_KEYS, 1000);
-String prefix = queryParams().get(QueryParams.PREFIX, "");
-String startAfter = queryParams().get(QueryParams.START_AFTER);
-
-Iterator ozoneKeyIterator = null;
-ContinueToken decodedToken = ContinueToken.decodeFromString(continueToken);
-OzoneBucket bucket = null;
-
+ Response handleGetRequest(S3RequestContext context, String bucketName)
+ throws IOException, OS3Exception {
+long startNanos = context.getStartNanos();
+PerformanceStringBuilder perf = context.getPerf();
try {
- maxKeys = validateMaxKeys(maxKeys);
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
- }
-
- // If continuation token and start after both are provided, then we
- // ignore start After
- String prevKey = continueToken != null ? decodedToken.getLastKey()
- : startAfter;
-
- // If shallow is true, only list immediate children
- // delimited by OZONE_URI_DELIMITER
- boolean shallow = listKeysShallowEnabled
- && OZONE_URI_DELIMITER.equals(delimiter);
-
- bucket = context.getVolume().getBucket(bucketName);
- S3Owner.verifyBucketOwnerCondition(getHeaders(), bucketName,
bucket.getOwner());
-
- ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);
-
+ return BucketListing.fromQueryParams(this, bucketName)
+ .buildResponse(startNanos, perf);
} catch (OMException ex) {
- getMetrics().updateGetBucketFailureStats(context.getStartNanos());
- if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
-// File not found, continue and send normal response with 0 keyCount
-LOG.debug("Key Not found prefix: {}", prefix);
- } else {
-throw ex;
+ getMetrics().updateGetBucketFailureStats(startNanos);
+ if (isAccessDenied(ex)) {
+throw newError(S3ErrorTable.ACCESS_DENIED, bucketName, ex);
}
+ throw ex;
} catch (Exception ex) {
- getMetrics().updateGetBucketFailureStats(context.getStartNanos());
+ getMetrics().updateGetBucketFailureStats(startNanos);
Review Comment:
nit: unnecessary change
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketListing.java:
##
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static
org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
+import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
+import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Objects;
+import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes;
+import org.apache.hadoop.ozone.s3.commontypes.EncodingTypeObject;
+import org.apache.hadoop.ozone.s3.exception.OS3Exception;
+import org.apache.hadoop.ozone.s3.exception.S3ErrorTable;
+import org.apache.hadoop.ozone.s3.util.ContinueToken;
+import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams;
+import org.s
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on PR #10045: URL: https://github.com/apache/ozone/pull/10045#issuecomment-4417879391 @adoroszlai @sreejasahithi just try to refactor the changes could you plz take a look to confirm the changes is right or not? thanks -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3216566206
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -72,7 +74,7 @@
* Bucket level rest endpoints.
*/
@Path("/{bucket}")
-public class BucketEndpoint extends BucketOperationHandler {
+public class BucketEndpoint extends EndpointBase {
Review Comment:
@adoroszlai thanks for the reminder!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
adoroszlai commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3131638119
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -72,7 +74,7 @@
* Bucket level rest endpoints.
*/
@Path("/{bucket}")
-public class BucketEndpoint extends BucketOperationHandler {
+public class BucketEndpoint extends EndpointBase {
Review Comment:
Please verify conflict resolution. The current state reverts
ab71c6af45f6a916eafbfdad9f33ab568a6487f5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
adoroszlai commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3121932411
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
-// The valid encodingType Values is "url"
+// Return empty response if no keys found
+// This path is reached when FILE_NOT_FOUND exception is caught and handled
+ListObjectResponse emptyResponse = new ListObjectResponse();
+emptyResponse.setName(bucketName);
+emptyResponse.setKeyCount(0);
+
+// Log audit entry for empty response to align with previous behavior
+long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+perf.appendCount(0);
+perf.appendOpLatencyNanos(opLatencyNs);
+auditReadSuccess(s3GAction, perf);
+
+return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+}
+
+return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
+private final String bucketName;
+private final String delimiter;
+private final String encodingType;
+private final String marker;
+private final int maxKeys;
+private final String prefix;
+private final String continueToken;
+private final String startAfter;
+private final String prevKey;
+private final boolean shallow;
+private final OzoneBucket bucket;
+private final Iterator ozoneKeyIterator;
+private final ContinueToken decodedToken;
+
+@SuppressWarnings("parameternumber")
+BucketListingContext(String bucketName, String delimiter, String
encodingType,
+ String marker, int maxKeys, String prefix,
String continueToken,
+ String startAfter, String prevKey, boolean
shallow,
+ OzoneBucket bucket, Iterator ozoneKeyIterator,
+ ContinueToken decodedToken) {
+ this.bucketName = bucketName;
+ this.delimiter = delimiter;
+ this.encodingType = encodingType;
+ this.marker = marker;
+ this.maxKeys = maxKeys;
+ this.prefix = prefix;
+ this.continueToken = continueToken;
+ this.startAfter = startAfter;
+ this.prevKey = prevKey;
+ this.shallow = shallow;
+ this.bucket = bucket;
+ this.ozoneKeyIterator = ozoneKeyIterator;
+ this.decodedToken = decodedToken;
+}
+
+// Getters
+public String getBucketName() {
+ return bucketName;
+}
+
+public String getDelimiter() {
+ return delimiter;
+}
+
+public String getEncodingType() {
+ return encodingType;
+}
+
+public String getMarker() {
+ return marker;
+}
+
+public int getMaxKeys() {
+ return maxKeys;
+}
+
+public String getPrefix() {
+ return prefix;
+}
+
+public String getContinueToken() {
+ return continueToken;
+}
+
+public String getStartAfter() {
+ return startAfter;
+}
+
+public String getPrevKey() {
+ return prevKey;
+}
+
+public boolean isShallow() {
+ return shallow;
+}
+
+public OzoneBucket getBucket() {
+ return bucket;
+}
+
+public Iterator getOzoneKeyIterator() {
+ return ozoneKeyIterator;
+}
+
+public ContinueToken getDecodedToken() {
+ return decodedToken;
+}
+ }
+
+ /**
+ * Validate and prepare parameters for bucket listing.
+ */
+ @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+ BucketListingContext validateAndPrepareParameters(
+ String bucketName, String delimiter, String encodingType, String marker,
+ int maxKeys, String prefix, String continueToken, String startAfter)
+ throws OS3Exception, IOException {
Review Comment:
Please move validation into `BucketListingContext`, which should perform it
on the values read from `queryParams`.
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -177,71 +164,242 @@ public Response get(
throw ex;
}
-// The valid encodingType Values is "url"
+// Return empty response if no keys found
+// This path is reached when FILE_NOT_FOUND exception is caught and handled
+ListObjectResponse emptyResponse = new ListObjectResponse();
+emptyResponse.setName(bucketName);
+emptyResponse.setKeyCount(0);
+
+// Log audit entry for empty response to align with previous behavior
+long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+perf.appendCount(0);
+perf.appendOpLatencyNanos(opLatencyNs);
+auditReadSuccess(s3GAction, perf);
+
+return Resp
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
sreejasahithi commented on PR #10045: URL: https://github.com/apache/ozone/pull/10045#issuecomment-4293658727 @rich7420 could you please resolve the conflicts in this branch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
rich7420 commented on PR #10045: URL: https://github.com/apache/ozone/pull/10045#issuecomment-4196488598 @sreejasahithi Thanks for catching these! I've removed the redundant comment and updated `initializeListObjectResponse` to properly use the capped listingContext.getMaxKeys(). The PR is updated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
sreejasahithi commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3039761983
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -134,31 +130,22 @@ public Response get(
return listMultipartUploads(bucketName, prefix, keyMarker,
uploadIdMarker, maxUploads);
}
- maxKeys = validateMaxKeys(maxKeys);
-
- if (prefix == null) {
-prefix = "";
- }
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
- }
+ // Actual bucket processing starts here
+ // Validate and prepare parameters
+ BucketListingContext listingContext = validateAndPrepareParameters(
+ bucketName, delimiter, encodingType, marker, maxKeys, prefix,
+ continueToken, startAfter);
- // If continuation token and start after both are provided, then we
- // ignore start After
- String prevKey = continueToken != null ? decodedToken.getLastKey()
- : startAfter;
+ // Initialize response object
+ ListObjectResponse response = initializeListObjectResponse(
+ bucketName, delimiter, encodingType, marker, maxKeys, prefix,
+ continueToken, startAfter);
Review Comment:
Here maxKeys is initialized from the query parameters (e.g., a user requests
5000), validateAndPrepareParameters is called. Inside this method, maxKeys is
capped via validateMaxKeys(maxKeys) (e.g., capped to 1000). This capped value
is stored inside the BucketListingContext, the maxKeys variable back in the
get() method is not updated. It remains 5000. initializeListObjectResponse is
called using the original maxKeys (5000).The response object now explicitly
states 5000.
I think if we pass listingContext.getMaxKeys() not the local `maxKeys` to
initializeListObjectResponse this issue would not be seen.
Could you confirm whether that’s intentional or something we should align?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-13532. Refactor BucketEndpoint.get() [ozone]
sreejasahithi commented on code in PR #10045:
URL: https://github.com/apache/ozone/pull/10045#discussion_r3039669232
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -177,11 +164,140 @@ public Response get(
throw ex;
}
-// The valid encodingType Values is "url"
+// Return empty response if no keys found
+// This path is reached when FILE_NOT_FOUND exception is caught and handled
+ListObjectResponse emptyResponse = new ListObjectResponse();
+emptyResponse.setName(bucketName);
+emptyResponse.setKeyCount(0);
+
+// Log audit entry for empty response to align with previous behavior
+long opLatencyNs = getMetrics().updateGetBucketSuccessStats(startNanos);
+perf.appendCount(0);
+perf.appendOpLatencyNanos(opLatencyNs);
+auditReadSuccess(s3GAction, perf);
+
+return Response.ok(emptyResponse).build();
+ }
+
+ private int validateMaxKeys(int maxKeys) throws OS3Exception {
+if (maxKeys < 0) {
+ throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be >= 0");
+}
+
+return Math.min(maxKeys, maxKeysLimit);
+ }
+
+ /**
+ * Context class to hold bucket listing parameters and state.
+ */
+ static class BucketListingContext {
+private final String bucketName;
+private final String delimiter;
+private final String encodingType;
+private final String marker;
+private final int maxKeys;
+private final String prefix;
+private final String continueToken;
+private final String startAfter;
+private final String prevKey;
+private final boolean shallow;
+private final OzoneBucket bucket;
+private final Iterator ozoneKeyIterator;
+private final ContinueToken decodedToken;
+
+@SuppressWarnings("parameternumber")
+BucketListingContext(String bucketName, String delimiter, String
encodingType,
+ String marker, int maxKeys, String prefix,
String continueToken,
+ String startAfter, String prevKey, boolean
shallow,
+ OzoneBucket bucket, Iterator ozoneKeyIterator,
+ ContinueToken decodedToken) {
+ this.bucketName = bucketName;
+ this.delimiter = delimiter;
+ this.encodingType = encodingType;
+ this.marker = marker;
+ this.maxKeys = maxKeys;
+ this.prefix = prefix;
+ this.continueToken = continueToken;
+ this.startAfter = startAfter;
+ this.prevKey = prevKey;
+ this.shallow = shallow;
+ this.bucket = bucket;
+ this.ozoneKeyIterator = ozoneKeyIterator;
+ this.decodedToken = decodedToken;
+}
+
+// Getters
+public String getBucketName() {
+ return bucketName;
+}
+
+public String getDelimiter() {
+ return delimiter;
+}
+
+public String getEncodingType() {
+ return encodingType;
+}
+
+public String getMarker() {
+ return marker;
+}
+
+public int getMaxKeys() {
+ return maxKeys;
+}
+
+public String getPrefix() {
+ return prefix;
+}
+
+public String getContinueToken() {
+ return continueToken;
+}
+
+public String getStartAfter() {
+ return startAfter;
+}
+
+public String getPrevKey() {
+ return prevKey;
+}
+
+public boolean isShallow() {
+ return shallow;
+}
+
+public OzoneBucket getBucket() {
+ return bucket;
+}
+
+public Iterator getOzoneKeyIterator() {
+ return ozoneKeyIterator;
+}
+
+public ContinueToken getDecodedToken() {
+ return decodedToken;
+}
+ }
+
+ /**
+ * Validate and prepare parameters for bucket listing.
+ */
+ @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+ BucketListingContext validateAndPrepareParameters(
+ String bucketName, String delimiter, String encodingType, String marker,
+ int maxKeys, String prefix, String continueToken, String startAfter)
+ throws OS3Exception, IOException {
+
+// If you specify the encoding-type request parameter, should return
encoded key name values
+// in the following response elements: Delimiter, Prefix, Key, and
StartAfter.
+// For detail refer:
+//
https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#AmazonS3-ListObjectsV2-response-EncodingType
Review Comment:
nit : this comment is repeated.
##
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##
@@ -134,31 +130,22 @@ public Response get(
return listMultipartUploads(bucketName, prefix, keyMarker,
uploadIdMarker, maxUploads);
}
- maxKeys = validateMaxKeys(maxKeys);
-
- if (prefix == null) {
-prefix = "";
- }
-
- // Assign marker to startAfter. for the compatibility of aws api v1
- if (startAfter == null && marker != null) {
-startAfter = marker;
-
