adoroszlai commented on code in PR #7753: URL: https://github.com/apache/ozone/pull/7753#discussion_r1929780495
########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpointBuilder.java: ########## @@ -0,0 +1,50 @@ +/* + * 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 org.apache.hadoop.ozone.s3.RequestIdentifier; + +/** + * Builder for BucketEndpoint. + */ +public class BucketEndpointBuilder extends + EndpointBuilder<BucketEndpoint, BucketEndpointBuilder> { + + private BucketEndpoint base; + + public BucketEndpointBuilder setBase(BucketEndpoint base) { + this.base = base; + return this; + } + + @Override + public BucketEndpoint build() { + BucketEndpoint endpoint = base != null ? base : new BucketEndpoint(); + if (getClient() != null) { + endpoint.setClient(getClient()); + } + if (getRequestId() == null) { + endpoint.setRequestIdentifier(new RequestIdentifier()); + } + endpoint.setRequestIdentifier(getRequestId()); + return endpoint; + } Review Comment: This base implementation of `build()` can also be moved to `EndpointBuilder`: ```java protected EndpointBuilder(Supplier<T> constructor) { this.constructor = constructor; ... } public T build() { T endpoint = base != null ? base : constructor.get(); if (ozoneClient != null) { endpoint.setClient(ozoneClient); } endpoint.setRequestIdentifier(identifier); return endpoint; } ``` (Since `this.identifier` is eagerly created, I think we can skip the conditional in `build()`.) Subclasses can pass the constructor as: ```java public ObjectEndpointBuilder() { super(ObjectEndpoint::new); } ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBuilder.java: ########## @@ -0,0 +1,98 @@ +/* + * 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 org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.s3.RequestIdentifier; + +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.HttpHeaders; + +/** + * Base builder class for S3 endpoints in tests. + * @param <T> Type of endpoint being built + * @param <B> Type of concrete builder (for method chaining) + */ +public abstract class EndpointBuilder<T, B extends EndpointBuilder<T, B>> { + private OzoneClient ozoneClient; + private OzoneConfiguration ozoneConfig; + private HttpHeaders httpHeaders; + private ContainerRequestContext requestContext; + private RequestIdentifier identifier; + + protected EndpointBuilder() { + this.ozoneConfig = new OzoneConfiguration(); + this.identifier = new RequestIdentifier(); + } + + @SuppressWarnings("unchecked") + public B setClient(OzoneClient newClient) { + this.ozoneClient = newClient; + return (B) this; + } Review Comment: After removing `<B>`: ```suggestion public EndpointBuilder<T> setClient(OzoneClient newClient) { this.ozoneClient = newClient; return this; } ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBuilder.java: ########## @@ -0,0 +1,98 @@ +/* + * 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 org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.s3.RequestIdentifier; + +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.HttpHeaders; + +/** + * Base builder class for S3 endpoints in tests. + * @param <T> Type of endpoint being built + * @param <B> Type of concrete builder (for method chaining) Review Comment: Since all `set...` methods are defined `EndpointBuilder`, I don't think we need to cast for chaining method calls. ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectGet.java: ########## @@ -88,12 +87,13 @@ public void init() throws OS3Exception, IOException { client = new OzoneClientStub(); client.getObjectStore().createS3Bucket(BUCKET_NAME); - rest = new ObjectEndpoint(); - rest.setClient(client); - rest.setOzoneConfiguration(new OzoneConfiguration()); headers = mock(HttpHeaders.class); - rest.setHeaders(headers); - rest.setRequestIdentifier(new RequestIdentifier()); + + rest = new ObjectEndpointBuilder() + .setClient(client) + .setConfig(new OzoneConfiguration()) Review Comment: ```suggestion ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpointBuilder.java: ########## @@ -0,0 +1,67 @@ +/* + * 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 org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.s3.RequestIdentifier; + +/** + * Builder for ObjectEndpoint in tests. + */ +public class ObjectEndpointBuilder extends + EndpointBuilder<ObjectEndpoint, ObjectEndpointBuilder> { + + private ObjectEndpoint base; + + public ObjectEndpointBuilder setBase(ObjectEndpoint base) { + this.base = base; + return this; + } + + @Override + public ObjectEndpoint build() { + ObjectEndpoint endpoint = base != null ? base : new ObjectEndpoint(); + + if (getClient() != null) { + endpoint.setClient(getClient()); + } + + if (getConfig() == null) { + endpoint.setOzoneConfiguration(new OzoneConfiguration()); + } + endpoint.setOzoneConfiguration(getConfig()); + + if (getRequestId() == null) { + endpoint.setRequestIdentifier(new RequestIdentifier()); + } + endpoint.setRequestIdentifier(getRequestId()); Review Comment: Using common build logic from parent: ```suggestion ObjectEndpoint endpoint = super.build(); final OzoneConfiguration config = getConfig(); endpoint.setOzoneConfiguration(config != null ? config : new OzoneConfiguration()); ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java: ########## @@ -20,8 +20,18 @@ package org.apache.hadoop.ozone.s3.endpoint; -import javax.ws.rs.core.HttpHeaders; -import javax.ws.rs.core.Response; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.hadoop.ozone.s3.util.S3Consts.COPY_SOURCE_HEADER; +import static org.apache.hadoop.ozone.s3.util.S3Consts.COPY_SOURCE_HEADER_RANGE; +import static org.apache.hadoop.ozone.s3.util.S3Consts.COPY_SOURCE_IF_MODIFIED_SINCE; +import static org.apache.hadoop.ozone.s3.util.S3Consts.COPY_SOURCE_IF_UNMODIFIED_SINCE; +import static org.apache.hadoop.ozone.s3.util.S3Consts.STORAGE_CLASS_HEADER; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + Review Comment: Please avoid moving around imports (especially when file is not changed otherwise). ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpointBuilder.java: ########## @@ -0,0 +1,50 @@ +/* + * 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 org.apache.hadoop.ozone.s3.RequestIdentifier; + +/** + * Builder for BucketEndpoint. + */ +public class BucketEndpointBuilder extends + EndpointBuilder<BucketEndpoint, BucketEndpointBuilder> { + + private BucketEndpoint base; + + public BucketEndpointBuilder setBase(BucketEndpoint base) { + this.base = base; + return this; + } Review Comment: `base` and `setBase` can be moved to `EndpointBuilder` as: ```java private T base; public B setBase(T base) { this.base = base; ... ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBuilder.java: ########## @@ -0,0 +1,98 @@ +/* + * 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 org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.s3.RequestIdentifier; + +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.HttpHeaders; + +/** + * Base builder class for S3 endpoints in tests. + * @param <T> Type of endpoint being built + * @param <B> Type of concrete builder (for method chaining) + */ +public abstract class EndpointBuilder<T, B extends EndpointBuilder<T, B>> { Review Comment: Let's add limit on `T`: ```suggestion public class EndpointBuilder<T extends EndpointBase> { ``` and make the class concrete. With the suggested changes `RootEndpointBuilder` and `BucketEndpointBuilder` will become "empty". So we can get rid of those two subclasses and add factory methods in `EndpointBuilder`: ```java public static EndpointBuilder<RootEndpoint> newRootEndpointBuilder() { return new EndpointBuilder<>(RootEndpoint::new); } public static EndpointBuilder<BucketEndpoint> newBucketEndpointBuilder() { return new EndpointBuilder<>(BucketEndpoint::new); } public static EndpointBuilder<ObjectEndpoint> newObjectEndpointBuilder() { return new ObjectEndpointBuilder(); } ``` -- 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]
