jojochuang commented on code in PR #8399:
URL: https://github.com/apache/ozone/pull/8399#discussion_r2078712240
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3StorageType.java:
##########
@@ -52,33 +54,13 @@ public ReplicationType getType() {
return type;
}
- /**
- * Get default S3StorageType for a new key to be uploaded.
- * This should align to the ozone cluster configuration.
- * @param config OzoneConfiguration
- * @return S3StorageType which wraps ozone replication type and factor
- */
- public static S3StorageType getDefault(ConfigurationSource config) {
- String replicationString = config.get(OzoneConfigKeys.OZONE_REPLICATION);
- ReplicationFactor configFactor;
- if (replicationString == null) {
- // if no config is set then let server take decision
- return null;
- }
- try {
- configFactor = ReplicationFactor.valueOf(
- Integer.parseInt(replicationString));
- } catch (NumberFormatException ex) {
- // conservatively defaults to STANDARD on wrong config value
- return STANDARD;
- }
- return configFactor == ReplicationFactor.ONE
- ? REDUCED_REDUNDANCY : STANDARD;
+ public String getEcReplicationString() {
+ return ecReplicationString;
}
public static S3StorageType fromReplicationConfig(ReplicationConfig config) {
if (config instanceof ECReplicationConfig) {
- return S3StorageType.STANDARD;
+ return STANDARD_IA;
Review Comment:
map EC type to STANDARD_IA.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -256,12 +257,13 @@ public Response put(
copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
storageType = headers.getHeaderString(STORAGE_CLASS_HEADER);
+ storageConfig = headers.getHeaderString(CUSTOM_METADATA_HEADER_PREFIX +
STORAGE_CONFIG_HEADER);
Review Comment:
curious: how will a client add the header to the request? is it supported by
s3 cli or hadoop aws cloud connector?
i wonder where we add this information in the developer doc.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -97,7 +98,7 @@ public abstract class EndpointBase implements Auditor {
private ContainerRequestContext context;
private Set<String> excludeMetadataFields =
- new HashSet<>(Arrays.asList(OzoneConsts.GDPR_FLAG));
+ new HashSet<>(Arrays.asList(OzoneConsts.GDPR_FLAG,
STORAGE_CONFIG_HEADER));
Review Comment:
what does this mean
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/util/TestS3Utils.java:
##########
@@ -141,7 +126,10 @@ public void
testResolveRepConfWhenUserPassedIsInvalidAndBucketDefaultNonEC()
public void testResolveRepConfWhenUserPassedIsInvalid() throws OS3Exception {
assertThrows(OS3Exception.class, () -> S3Utils.
resolveS3ClientSideReplicationConfig(
- "INVALID", ratis3ReplicationConfig, ratis1ReplicationConfig));
+ "INVALID", null, RATIS3REPLICATIONCONFIG,
RATIS1REPLICATIONCONFIG));
+ assertThrows(OS3Exception.class, () -> S3Utils.
+ resolveS3ClientSideReplicationConfig(S3StorageType.STANDARD_IA.name(),
+ "INVALID", RATIS3REPLICATIONCONFIG, RATIS1REPLICATIONCONFIG));
Review Comment:
also invalid s3StorageTypeHeader
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Utils.java:
##########
@@ -63,64 +64,44 @@ private S3Utils() {
*
* @param s3StorageTypeHeader - s3 user passed storage type
* header.
- * @param clientConfiguredReplConfig - Client side configured replication
- * config.
* @param bucketReplConfig - server side bucket default replication
* config.
+ * @param clientConfiguredReplConfig - Client side configured replication
+ * config.
* @return client resolved replication config.
*/
public static ReplicationConfig resolveS3ClientSideReplicationConfig(
- String s3StorageTypeHeader, ReplicationConfig clientConfiguredReplConfig,
+ String s3StorageTypeHeader, String s3StorageConfigHeader,
+ ReplicationConfig clientConfiguredReplConfig,
ReplicationConfig bucketReplConfig)
throws OS3Exception {
- ReplicationConfig clientDeterminedReplConfig = null;
- // Let's map the user provided s3 storage type header to ozone s3 storage
- // type.
- S3StorageType s3StorageType = null;
- if (s3StorageTypeHeader != null && !s3StorageTypeHeader.equals("")) {
- s3StorageType = toS3StorageType(s3StorageTypeHeader);
+ // If user provided s3 storage type header is not null then map it
+ // to ozone replication config
+ if (!StringUtils.isEmpty(s3StorageTypeHeader)) {
+ return toReplicationConfig(s3StorageTypeHeader, s3StorageConfigHeader);
}
- boolean isECBucket = bucketReplConfig != null && bucketReplConfig
- .getReplicationType() == HddsProtos.ReplicationType.EC;
-
- // if bucket replication config configured with EC, we will give high
- // preference to server side bucket defaults.
- // Why we give high preference to EC is, there is no way for file system
- // interfaces to pass EC replication. So, if one configures EC at bucket,
- // we consider EC to take preference. in short, keys created from file
- // system under EC bucket will always be EC'd.
- if (isECBucket) {
- // if bucket is EC, don't bother client provided configs, let's pass
- // bucket config.
- clientDeterminedReplConfig = bucketReplConfig;
- } else {
- // Let's validate the client side available replication configs.
- boolean isUserPassedReplicationInSupportedList =
- s3StorageType != null && (s3StorageType.getFactor()
- .getValue() == ReplicationFactor.ONE.getValue() || s3StorageType
- .getFactor().getValue() == ReplicationFactor.THREE.getValue());
- if (isUserPassedReplicationInSupportedList) {
- clientDeterminedReplConfig = ReplicationConfig.fromProtoTypeAndFactor(
- ReplicationType.toProto(s3StorageType.getType()),
- ReplicationFactor.toProto(s3StorageType.getFactor()));
- } else {
- // API passed replication number is not in supported replication list.
- // So, let's use whatever available in client side configured.
- // By default it will be null, so server will use server defaults.
- clientDeterminedReplConfig = clientConfiguredReplConfig;
- }
- }
- return clientDeterminedReplConfig;
+ // If client configured replication config is null then default to bucket
replication
+ // otherwise default to server side default replication config.
+ return (clientConfiguredReplConfig != null) ?
+ clientConfiguredReplConfig : bucketReplConfig;
}
- public static S3StorageType toS3StorageType(String storageType)
+ public static ReplicationConfig toReplicationConfig(String storageType,
String storageConfig)
Review Comment:
no test
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/util/TestS3Utils.java:
##########
@@ -141,7 +126,10 @@ public void
testResolveRepConfWhenUserPassedIsInvalidAndBucketDefaultNonEC()
public void testResolveRepConfWhenUserPassedIsInvalid() throws OS3Exception {
assertThrows(OS3Exception.class, () -> S3Utils.
resolveS3ClientSideReplicationConfig(
- "INVALID", ratis3ReplicationConfig, ratis1ReplicationConfig));
+ "INVALID", null, RATIS3REPLICATIONCONFIG,
RATIS1REPLICATIONCONFIG));
+ assertThrows(OS3Exception.class, () -> S3Utils.
+ resolveS3ClientSideReplicationConfig(S3StorageType.STANDARD_IA.name(),
+ "INVALID", RATIS3REPLICATIONCONFIG, RATIS1REPLICATIONCONFIG));
Review Comment:
shall we also verify for invalid bucketReplConfig?
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
