vinothchandar commented on code in PR #12954: URL: https://github.com/apache/hudi/pull/12954#discussion_r2031572623
########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockService.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.hudi.client.transaction.lock.models.ConditionalWriteLockData; +import org.apache.hudi.client.transaction.lock.models.ConditionalWriteLockFile; +import org.apache.hudi.client.transaction.lock.models.LockGetResult; +import org.apache.hudi.client.transaction.lock.models.LockUpdateResult; +import org.apache.hudi.common.util.collection.Pair; + +import java.util.function.Supplier; + +/** + * Defines a contract for a service which should be able to perform conditional writes to object storage. + * It expects to be interacting with a single lock file per context (table), and will be competing with other instances + * to perform writes, so it should handle these cases accordingly (using conditional writes). + */ +public interface ConditionalWriteLockService extends AutoCloseable { Review Comment: naming: just `ConditionalWriteLock` ? Service implies sth is started/closed and there is a lifecycle. also should this be called just "StorageLock" - thats the main difference compared to say ZK. that this is implemented right on top of Hudi's backing storage? "Writer" indicates that this is how we update/write to the underlying lock ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/models/ConditionalWriteLockFile.java: ########## @@ -0,0 +1,137 @@ +/* + * 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.models; + +import org.apache.hudi.exception.HoodieIOException; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; + +public class ConditionalWriteLockFile { + + private final ConditionalWriteLockData data; + private final String versionId; + + // Get a custom object mapper. See ConditionalWriteLockData for required properties. + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper() + // This allows us to add new properties down the line. + .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + // Should not let validUntil or expired be null. + .enable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES); + + /** + * Initializes a ConditionalWriteLockFile using the data and unique versionId. + * + * @param data The data in the lock file. + * @param versionId The version of this lock file. Used to ensure consistency through conditional writes. + */ + public ConditionalWriteLockFile(ConditionalWriteLockData data, String versionId) { + if (data == null) { Review Comment: use `ValidationUtils..` methods? ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/models/ConditionalWriteLockData.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.models; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Pojo for conditional write lock provider. + */ +public class ConditionalWriteLockData { Review Comment: if we rename that, then we rename all these pojos - ConditionalWriteLockxxx to StorageLockxxx ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockService.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.hudi.client.transaction.lock.models.ConditionalWriteLockData; +import org.apache.hudi.client.transaction.lock.models.ConditionalWriteLockFile; +import org.apache.hudi.client.transaction.lock.models.LockGetResult; +import org.apache.hudi.client.transaction.lock.models.LockUpdateResult; +import org.apache.hudi.common.util.collection.Pair; + +import java.util.function.Supplier; + +/** + * Defines a contract for a service which should be able to perform conditional writes to object storage. + * It expects to be interacting with a single lock file per context (table), and will be competing with other instances + * to perform writes, so it should handle these cases accordingly (using conditional writes). + */ +public interface ConditionalWriteLockService extends AutoCloseable { + /** + * Tries once to create or update a lock file. + * @param newLockData The new data to update the lock file with. + * @param previousLockFile The previous lock file, use this to conditionally update the lock file. + * @return A pair containing the result state and the new lock file (if successful) + */ + Pair<LockUpdateResult, ConditionalWriteLockFile> tryCreateOrUpdateLockFile( + ConditionalWriteLockData newLockData, + ConditionalWriteLockFile previousLockFile); + + /** + * Tries to create or update a lock file while retrying N times. + * All non pre-condition failure related errors should be retried. + * @param newLockDataSupplier The new data supplier + * @param previousLockFile The previous lock file + * @param retryCount Number of retries to attempt + * @return A pair containing the result state and the new lock file (if successful) + */ + Pair<LockUpdateResult, ConditionalWriteLockFile> tryCreateOrUpdateLockFileWithRetry( + Supplier<ConditionalWriteLockData> newLockDataSupplier, + ConditionalWriteLockFile previousLockFile, + long retryCount); + + /** + * Gets the current lock file. + * @return The lock retrieve result and the current lock file if successfully retrieved. + * */ + Pair<LockGetResult, ConditionalWriteLockFile> getCurrentLockFile(); Review Comment: rename: readCurrentLockFile() ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/models/LockGetResult.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.models; + +public enum LockGetResult { + // Lock file does not exist + NOT_EXISTS, Review Comment: add a member for the enum code.. i.e NOT_EXISTS has code `0`. this way no one accidentally adds an enum in the middle. -- 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]
