nastra commented on code in PR #9048: URL: https://github.com/apache/iceberg/pull/9048#discussion_r1401825158
########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestCosClientFactories.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.*; + +public class TestCosClientFactories { + + @Test + public void testLoadDefault() { + assertThat(CosClientFactories.defaultFactory()).isEqualTo(CosClientFactories.defaultFactory()); + + CosClientFactory defaultFactory = CosClientFactories.from(Maps.newHashMap()); + assertThat(defaultFactory).isInstanceOf(CosClientFactories.DefaultCosClientFactory.class); + assertThat(defaultFactory.cosProperties().accessKeyId()).isNull(); + + CosClientFactory defaultFactoryWithConfig = + CosClientFactories.from(ImmutableMap.of(CosProperties.COS_USERINFO_SECRET_KEY, "key")); + assertThat(defaultFactoryWithConfig).isInstanceOf(CosClientFactories.DefaultCosClientFactory.class); + assertThat("key").isEqualTo(defaultFactoryWithConfig.cosProperties().accessKeySecret()); + assertThatThrownBy(() -> + CosClientFactories.from( + ImmutableMap.of(CosProperties.CLIENT_FACTORY, "UnknownCosFactories"))) + .isInstanceOf(IllegalArgumentException.class); Review Comment: this should have a `.hasMessage(...)` check ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosURI.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.internal.BucketNameUtils; +import java.util.Set; +import org.apache.hadoop.fs.Path; +import org.apache.iceberg.exceptions.ValidationException; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; + +public class CosURI { + private static final String SCHEME_DELIM = "://"; + private static final String PATH_DELIM = "/"; + private static final String QUERY_DELIM = "\\?"; + private static final String FRAGMENT_DELIM = "#"; + private static final Set<String> VALID_SCHEMES = ImmutableSet.of("cos", "cosn"); + private final String location; + private final String bucket; + private final String key; + + public CosURI(String location) { + Preconditions.checkNotNull(location, "Cos location cannot be null."); Review Comment: ```suggestion `Preconditions.checkArgument(null != location, "Invalid Cos location: null"); ``` ########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestCosFileIO.java: ########## @@ -0,0 +1,166 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.Random; +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.hadoop.conf.Configuration; +import org.apache.iceberg.CatalogUtil; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.io.ByteStreams; +import org.apache.iceberg.util.SerializableSupplier; +import org.apache.iceberg.util.SerializationUtil; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.*; + +public class TestCosFileIO { + private static final String COS_IMPL_CLASS = CosFileIO.class.getName(); + private final Configuration conf = new Configuration(); + private final Random random = ThreadLocalRandom.current(); + + private FileIO fileIO; + + @BeforeEach + public void beforeFile() { + fileIO = new CosFileIO((SerializableSupplier<COS>) TestUtility::cos); + } + + @AfterEach + public void afterFile() { + if (fileIO != null) { + fileIO.close(); + } + } + + @Test + public void testOutputFile() throws IOException { + String location = randomLocation(); + int dataSize = 1024 * 10; + byte[] data = randomData(dataSize); + + OutputFile out = fileIO().newOutputFile(location); + writeCosData(out, data); + + CosURI uri = new CosURI(location); + + assertThat(TestUtility.cos().doesObjectExist(uri.bucket(), uri.key())) + .as("Cos file should exist").isTrue(); + assertThat(location).as("Should have expected location").isEqualTo(out.location()); + assertThat(dataSize).as("Should have expected length").isEqualTo(cosDataLength(uri)); + assertThat(data).as("Should have expected content").containsExactly(cosDataContent(uri, dataSize)); + } + + @Test + public void testInputFile() throws IOException { + String location = randomLocation(); + InputFile in = fileIO().newInputFile(location); + + assertThat(in.exists()).as("Cos file should not exist").isFalse(); + + int dataSize = 1024 * 10; + byte[] data = randomData(dataSize); + OutputFile out = fileIO().newOutputFile(location); + writeCosData(out, data); + + assertThat(in.exists()).as("Cos file should exist").isTrue(); + assertThat(location).as("Should have expected location").isEqualTo(in.location()); + assertThat(dataSize).as("Should have expected length").isEqualTo(in.getLength()); + assertThat(data).as("Should have expected content").containsExactly(inFileContent(in, dataSize)); + } + + @Test + public void testDeleteFile() throws IOException { + String location = randomLocation(); + int dataSize = 1024 * 10; + byte[] data = randomData(dataSize); + OutputFile out = fileIO().newOutputFile(location); + writeCosData(out, data); + + InputFile in = fileIO().newInputFile(location); + assertThat(in.exists()).isTrue(); + fileIO().deleteFile(in); + assertThat(fileIO().newInputFile(location).exists()).isFalse(); + } + + @Test + public void testLoadFileIO() { + FileIO file = CatalogUtil.loadFileIO(COS_IMPL_CLASS, ImmutableMap.of(), conf); + assertThat(file instanceof CosFileIO).isTrue(); Review Comment: ```suggestion assertThat(file).isInstanceOf(CosFileIO.class); ``` ########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestUtility.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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import com.qcloud.cos.COSClient; +import com.qcloud.cos.ClientConfig; +import com.qcloud.cos.auth.BasicCOSCredentials; +import com.qcloud.cos.auth.COSCredentials; +import com.qcloud.cos.region.Region; + +public class TestUtility { Review Comment: I'm not a fan of such test utility classes. I think a better approach would be to have a proper [JUnit5 test extension](https://junit.org/junit5/docs/current/user-guide/#extensions), that inits what's required for a particular test. A good & simple example of such an extension is https://github.com/adobe/S3Mock/blob/main/testsupport/junit5/src/main/java/com/adobe/testing/s3mock/junit5/S3MockExtension.java ########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestCosFileIO.java: ########## @@ -0,0 +1,166 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.Random; +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.hadoop.conf.Configuration; +import org.apache.iceberg.CatalogUtil; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.io.ByteStreams; +import org.apache.iceberg.util.SerializableSupplier; +import org.apache.iceberg.util.SerializationUtil; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.*; + +public class TestCosFileIO { + private static final String COS_IMPL_CLASS = CosFileIO.class.getName(); + private final Configuration conf = new Configuration(); Review Comment: no need to define this at the class level when it's only being used in a single test method ########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestCosInputFile.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import static org.mockito.AdditionalAnswers.delegatesTo; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import com.qcloud.cos.COS; +import com.qcloud.cos.model.ObjectMetadata; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.util.Random; +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.iceberg.exceptions.ValidationException; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.SeekableInputStream; +import org.apache.iceberg.metrics.MetricsContext; +import org.apache.iceberg.relocated.com.google.common.io.ByteStreams; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.*; + +public class TestCosInputFile { + private final COS cosMock = mock(COS.class, delegatesTo(TestUtility.cos())); + Review Comment: unnecessary newline ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/BaseCosFile.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import com.qcloud.cos.exception.CosServiceException; +import com.qcloud.cos.model.ObjectMetadata; +import org.apache.iceberg.metrics.MetricsContext; +import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; + +abstract class BaseCosFile { + private final COS client; + private final CosURI uri; + private CosProperties cosProperties; Review Comment: can be final ########## tencent/src/test/java/org/apache/iceberg/tencentcloud/cos/TestCosInputStream.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import com.qcloud.cos.model.ObjectMetadata; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.util.Arrays; +import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.iceberg.io.SeekableInputStream; +import org.apache.iceberg.relocated.com.google.common.io.ByteStreams; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.*; + +public class TestCosInputStream { + private final Random random = ThreadLocalRandom.current(); + Review Comment: newline ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosProperties.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import java.io.Serializable; +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.util.PropertyUtil; + +public class CosProperties implements Serializable { + public static final String COS_REGION = "fs.cos.bucket.region"; + + public static final String COS_APPID_KEY = "fs.cos.userinfo.appid"; + + public static final String COS_USERINFO_SECRET_ID = "fs.cos.userinfo.secretId"; + + public static final String COS_USERINFO_SECRET_KEY = "fs.cos.userinfo.secretKey"; + + public static final String CLIENT_FACTORY = "client.factory-impl"; + + public static final String COS_TMP_DIR = "fs.cos.tmp.dir"; Review Comment: maybe this should be called `COS_STAGING_DIR` rather than tmp dir? ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosProperties.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import java.io.Serializable; +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.util.PropertyUtil; + +public class CosProperties implements Serializable { + public static final String COS_REGION = "fs.cos.bucket.region"; + + public static final String COS_APPID_KEY = "fs.cos.userinfo.appid"; + + public static final String COS_USERINFO_SECRET_ID = "fs.cos.userinfo.secretId"; + + public static final String COS_USERINFO_SECRET_KEY = "fs.cos.userinfo.secretKey"; + + public static final String CLIENT_FACTORY = "client.factory-impl"; + + public static final String COS_TMP_DIR = "fs.cos.tmp.dir"; + + public static final String DEFAULT_TMP_DIR = "/tmp/hadoop_cos"; Review Comment: I think `System.getProperty("java.io.tmpdir")` would be better here than just hardcoding to `/tmp` ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosURI.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.internal.BucketNameUtils; +import java.util.Set; +import org.apache.hadoop.fs.Path; +import org.apache.iceberg.exceptions.ValidationException; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; + +public class CosURI { + private static final String SCHEME_DELIM = "://"; + private static final String PATH_DELIM = "/"; + private static final String QUERY_DELIM = "\\?"; + private static final String FRAGMENT_DELIM = "#"; + private static final Set<String> VALID_SCHEMES = ImmutableSet.of("cos", "cosn"); + private final String location; + private final String bucket; + private final String key; + + public CosURI(String location) { + Preconditions.checkNotNull(location, "Cos location cannot be null."); Review Comment: I'm aware that `S3URI` still uses `checkNotNull`, but for newly added code we now typically throw IAE (see also `GCSLocation` as an example) ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosProperties.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import java.io.Serializable; +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.util.PropertyUtil; + +public class CosProperties implements Serializable { + public static final String COS_REGION = "fs.cos.bucket.region"; + + public static final String COS_APPID_KEY = "fs.cos.userinfo.appid"; Review Comment: this doesn't seem to be used anywhere ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/BaseCosFile.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import com.qcloud.cos.exception.CosServiceException; +import com.qcloud.cos.model.ObjectMetadata; +import org.apache.iceberg.metrics.MetricsContext; +import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; + +abstract class BaseCosFile { + private final COS client; + private final CosURI uri; + private CosProperties cosProperties; + private ObjectMetadata metadata; + private final MetricsContext metrics; + + BaseCosFile(COS client, CosURI uri, CosProperties cosProperties, MetricsContext metrics) { + this.client = client; + this.uri = uri; + this.cosProperties = cosProperties; + this.metrics = metrics; + } + + public String location() { + return uri.location(); + } + + public COS client() { + return client; + } + + public CosURI uri() { + return uri; + } + + public CosProperties cosProperties() { + return cosProperties; + } + + public boolean exists() { + try { + return objectMetadata() != null; + } catch (CosServiceException e) { + if (e.getStatusCode() == 404) { + return false; + } else { + throw e; // return null if 404 Not Found, otherwise rethrow Review Comment: this comment seems confusing, I don't see where this is returning `null` ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosFileIO.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.iceberg.common.DynConstructors; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.metrics.MetricsContext; +import org.apache.iceberg.util.SerializableSupplier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class CosFileIO implements FileIO { + private static final Logger LOG = LoggerFactory.getLogger(CosFileIO.class); + private static final String DEFAULT_METRICS_IMPL = + "org.apache.iceberg.hadoop.HadoopMetricsContext"; + + private SerializableSupplier<COS> cosSupplier; + private CosProperties cosProperties; + private transient volatile COS client; + private MetricsContext metrics = MetricsContext.nullMetrics(); + private final AtomicBoolean isResourceClosed = new AtomicBoolean(false); + + public CosFileIO() {} + + public CosFileIO(SerializableSupplier<COS> cosSupplier) { + this.cosSupplier = cosSupplier; + this.cosProperties = new CosProperties(); + } + + @Override + public InputFile newInputFile(String path) { + return new CosInputFile(client(), new CosURI(path), cosProperties, metrics); + } + + @Override + public OutputFile newOutputFile(String path) { + return new CosOutputFile(client(), new CosURI(path), cosProperties, metrics); + } + + @Override + public void deleteFile(String path) { + CosURI location = new CosURI(path); + client().deleteObject(location.bucket(), location.key()); + } + + private COS client() { + if (client == null) { + synchronized (this) { + if (client == null) { + client = cosSupplier.get(); + } + } + } + return client; + } + + @Override + public void initialize(Map<String, String> properties) { + CosClientFactory factory = CosClientFactories.from(properties); + this.cosProperties = factory.cosProperties(); + this.cosSupplier = factory::newCosClient; + + // Report Hadoop metrics if Hadoop is available + try { + DynConstructors.Ctor<MetricsContext> ctor = + DynConstructors.builder(MetricsContext.class) + .hiddenImpl(DEFAULT_METRICS_IMPL, String.class) + .buildChecked(); + MetricsContext context = ctor.newInstance("cos"); + context.initialize(properties); + this.metrics = context; + } catch (NoClassDefFoundError | NoSuchMethodException | ClassCastException e) { + LOG.warn( + "Unable to load metrics class: '{}', falling back to null metrics", + DEFAULT_METRICS_IMPL, + e); + } + } + + @Override + public void close() { + // handles concurrent calls to close() + isResourceClosed.compareAndSet(false, true); Review Comment: also you might want to compare how `S3FileIO` uses `isResourceClosed` ########## tencent/src/main/java/org/apache/iceberg/tencentcloud/cos/CosFileIO.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.iceberg.tencentcloud.cos; + +import com.qcloud.cos.COS; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.iceberg.common.DynConstructors; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.metrics.MetricsContext; +import org.apache.iceberg.util.SerializableSupplier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class CosFileIO implements FileIO { + private static final Logger LOG = LoggerFactory.getLogger(CosFileIO.class); + private static final String DEFAULT_METRICS_IMPL = + "org.apache.iceberg.hadoop.HadoopMetricsContext"; + + private SerializableSupplier<COS> cosSupplier; + private CosProperties cosProperties; + private transient volatile COS client; + private MetricsContext metrics = MetricsContext.nullMetrics(); + private final AtomicBoolean isResourceClosed = new AtomicBoolean(false); + + public CosFileIO() {} + + public CosFileIO(SerializableSupplier<COS> cosSupplier) { + this.cosSupplier = cosSupplier; + this.cosProperties = new CosProperties(); + } + + @Override + public InputFile newInputFile(String path) { + return new CosInputFile(client(), new CosURI(path), cosProperties, metrics); + } + + @Override + public OutputFile newOutputFile(String path) { + return new CosOutputFile(client(), new CosURI(path), cosProperties, metrics); + } + + @Override + public void deleteFile(String path) { + CosURI location = new CosURI(path); + client().deleteObject(location.bucket(), location.key()); + } + + private COS client() { + if (client == null) { + synchronized (this) { + if (client == null) { + client = cosSupplier.get(); + } + } + } + return client; + } + + @Override + public void initialize(Map<String, String> properties) { + CosClientFactory factory = CosClientFactories.from(properties); + this.cosProperties = factory.cosProperties(); + this.cosSupplier = factory::newCosClient; + + // Report Hadoop metrics if Hadoop is available + try { + DynConstructors.Ctor<MetricsContext> ctor = + DynConstructors.builder(MetricsContext.class) + .hiddenImpl(DEFAULT_METRICS_IMPL, String.class) + .buildChecked(); + MetricsContext context = ctor.newInstance("cos"); + context.initialize(properties); + this.metrics = context; + } catch (NoClassDefFoundError | NoSuchMethodException | ClassCastException e) { + LOG.warn( + "Unable to load metrics class: '{}', falling back to null metrics", + DEFAULT_METRICS_IMPL, + e); + } + } + + @Override + public void close() { + // handles concurrent calls to close() + isResourceClosed.compareAndSet(false, true); Review Comment: I think this is missing closing of the client? -- 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...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
