yanghua commented on code in PR #6071: URL: https://github.com/apache/hudi/pull/6071#discussion_r922643570
########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java: ########## @@ -0,0 +1,151 @@ +/* + * 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.hudi.client.transaction.lock; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hudi.common.config.LockConfiguration; +import org.apache.hudi.common.fs.FSUtils; +import org.apache.hudi.common.lock.LockProvider; +import org.apache.hudi.common.lock.LockState; +import org.apache.hudi.common.table.HoodieTableMetaClient; +import org.apache.hudi.common.util.StringUtils; +import org.apache.hudi.common.util.ValidationUtils; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.exception.HoodieIOException; +import org.apache.hudi.exception.HoodieLockException; +import org.apache.log4j.LogManager; +import org.apache.log4j.Logger; + +import java.io.IOException; +import java.io.Serializable; +import java.util.concurrent.TimeUnit; + +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_EXPIRE_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_PATH_PROP_KEY; + +/** + * A FileSystem based lock. This {@link LockProvider} implementation allows to lock table operations + * using DFS. Users might need to manually clean the Locker's path if writeClient crash and never run again. + * NOTE: This only works for DFS with atomic create/delete operation + */ +public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable { + + private static final Logger LOG = LogManager.getLogger(FileSystemBasedLockProvider.class); + + private static final String LOCK_FILE_NAME = "lock"; + + private final int lockTimeoutMinutes; + private transient FileSystem fs; + private transient Path lockFile; + protected LockConfiguration lockConfiguration; + + public FileSystemBasedLockProvider(final LockConfiguration lockConfiguration, final Configuration configuration) { + checkRequiredProps(lockConfiguration); + this.lockConfiguration = lockConfiguration; + String lockDirectory = lockConfiguration.getConfig().getString(FILESYSTEM_LOCK_PATH_PROP_KEY, null); + if (StringUtils.isNullOrEmpty(lockDirectory)) { + lockDirectory = lockConfiguration.getConfig().getString(HoodieWriteConfig.BASE_PATH.key(), null) + + Path.SEPARATOR + HoodieTableMetaClient.METAFOLDER_NAME; + } + this.lockTimeoutMinutes = lockConfiguration.getConfig().getInteger(FILESYSTEM_LOCK_EXPIRE_PROP_KEY); + this.lockFile = new Path(lockDirectory + Path.SEPARATOR + LOCK_FILE_NAME); + this.fs = FSUtils.getFs(this.lockFile.toString(), configuration); + } + + @Override + public void close() { + synchronized (LOCK_FILE_NAME) { + try { + fs.delete(this.lockFile, true); + } catch (IOException e) { + throw new HoodieLockException(generateLogStatement(LockState.FAILED_TO_RELEASE), e); + } + } + } + + @Override + public boolean tryLock(long time, TimeUnit unit) { + try { + synchronized (LOCK_FILE_NAME) { + if (fs.exists(this.lockFile)) { + // Check whether lock is already expired or not, if so try to delete lock file + if (lockTimeoutMinutes != 0 && checkIfExpired()) { Review Comment: move `lockTimeoutMinutes != 0` into `checkIfExpired ` looks better? ########## hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/TestFileBasedLockProvider.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.hudi.client; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdfs.MiniDFSCluster; +import org.apache.hudi.client.transaction.lock.FileSystemBasedLockProvider; +import org.apache.hudi.common.config.LockConfiguration; +import org.apache.hudi.common.testutils.minicluster.HdfsTestService; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.exception.HoodieLockException; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_EXPIRE_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_PATH_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_NUM_RETRIES_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY; + +public class TestFileBasedLockProvider { + private static HdfsTestService hdfsTestService; + private static MiniDFSCluster dfsCluster; + private static LockConfiguration lockConfiguration; + private static Configuration hadoopConf; + + @BeforeAll + public static void setup() throws IOException { + hdfsTestService = new HdfsTestService(); + dfsCluster = hdfsTestService.start(true); + hadoopConf = dfsCluster.getFileSystem().getConf(); + + Properties properties = new Properties(); + properties.setProperty(FILESYSTEM_LOCK_PATH_PROP_KEY, "/tmp/"); + properties.setProperty(FILESYSTEM_LOCK_EXPIRE_PROP_KEY, "1"); + properties.setProperty(LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY, "1000"); + properties.setProperty(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS_PROP_KEY, "1000"); + properties.setProperty(LOCK_ACQUIRE_NUM_RETRIES_PROP_KEY, "3"); + lockConfiguration = new LockConfiguration(properties); + } + + @AfterAll + public static void cleanUpAfterAll() throws IOException { + Path workDir = dfsCluster.getFileSystem().getWorkingDirectory(); + FileSystem fs = workDir.getFileSystem(hdfsTestService.getHadoopConf()); + fs.delete(new Path("/tmp/lock"), true); + fs.delete(new Path("/tmp/.hoodie/lock"), true); + if (hdfsTestService != null) { + hdfsTestService.stop(); + hdfsTestService = null; + } + } + + @AfterEach + public void cleanUpAfterEach() throws IOException { + Path workDir = dfsCluster.getFileSystem().getWorkingDirectory(); + FileSystem fs = workDir.getFileSystem(hdfsTestService.getHadoopConf()); + fs.delete(new Path("/tmp/lock"), true); + } + + @Test + public void testAcquireLock() { + FileSystemBasedLockProvider fileBasedLockProvider = new FileSystemBasedLockProvider(lockConfiguration, hadoopConf); + Assertions.assertTrue(fileBasedLockProvider.tryLock(lockConfiguration.getConfig() + .getLong(LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY), TimeUnit.MILLISECONDS)); + fileBasedLockProvider.unlock(); + } + + @Test + public void testAcquireLockWithDefaultPath() { + lockConfiguration.getConfig().remove(FILESYSTEM_LOCK_PATH_PROP_KEY); + lockConfiguration.getConfig().setProperty(HoodieWriteConfig.BASE_PATH.key(), "/tmp/"); + FileSystemBasedLockProvider fileBasedLockProvider = new FileSystemBasedLockProvider(lockConfiguration, hadoopConf); + Assertions.assertTrue(fileBasedLockProvider.tryLock(lockConfiguration.getConfig() + .getLong(LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY), TimeUnit.MILLISECONDS)); + fileBasedLockProvider.unlock(); + lockConfiguration.getConfig().setProperty(FILESYSTEM_LOCK_PATH_PROP_KEY, "/tmp/"); + } + + @Test + public void testUnLock() { + FileSystemBasedLockProvider fileBasedLockProvider = new FileSystemBasedLockProvider(lockConfiguration, hadoopConf); + Assertions.assertTrue(fileBasedLockProvider.tryLock(lockConfiguration.getConfig() + .getLong(LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY), TimeUnit.MILLISECONDS)); + fileBasedLockProvider.unlock(); + fileBasedLockProvider.tryLock(lockConfiguration.getConfig() Review Comment: assert if the result of `tryLock` is true? ########## hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/TestFileBasedLockProvider.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.hudi.client; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdfs.MiniDFSCluster; +import org.apache.hudi.client.transaction.lock.FileSystemBasedLockProvider; +import org.apache.hudi.common.config.LockConfiguration; +import org.apache.hudi.common.testutils.minicluster.HdfsTestService; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.exception.HoodieLockException; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_EXPIRE_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.FILESYSTEM_LOCK_PATH_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_NUM_RETRIES_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS_PROP_KEY; +import static org.apache.hudi.common.config.LockConfiguration.LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY; + +public class TestFileBasedLockProvider { + private static HdfsTestService hdfsTestService; + private static MiniDFSCluster dfsCluster; + private static LockConfiguration lockConfiguration; + private static Configuration hadoopConf; + + @BeforeAll + public static void setup() throws IOException { + hdfsTestService = new HdfsTestService(); + dfsCluster = hdfsTestService.start(true); + hadoopConf = dfsCluster.getFileSystem().getConf(); + + Properties properties = new Properties(); + properties.setProperty(FILESYSTEM_LOCK_PATH_PROP_KEY, "/tmp/"); + properties.setProperty(FILESYSTEM_LOCK_EXPIRE_PROP_KEY, "1"); + properties.setProperty(LOCK_ACQUIRE_WAIT_TIMEOUT_MS_PROP_KEY, "1000"); + properties.setProperty(LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS_PROP_KEY, "1000"); + properties.setProperty(LOCK_ACQUIRE_NUM_RETRIES_PROP_KEY, "3"); + lockConfiguration = new LockConfiguration(properties); + } + + @AfterAll + public static void cleanUpAfterAll() throws IOException { + Path workDir = dfsCluster.getFileSystem().getWorkingDirectory(); + FileSystem fs = workDir.getFileSystem(hdfsTestService.getHadoopConf()); + fs.delete(new Path("/tmp/lock"), true); + fs.delete(new Path("/tmp/.hoodie/lock"), true); Review Comment: Can we just delete the `/tmp` dir recursively? -- 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]
