yihua commented on code in PR #13292:
URL: https://github.com/apache/hudi/pull/13292#discussion_r2112939621
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieLockConfig.java:
##########
@@ -351,7 +351,11 @@ public HoodieLockConfig.Builder
withHeartbeatIntervalInMillis(Long intervalInMil
}
public HoodieLockConfig.Builder
withConflictResolutionStrategy(ConflictResolutionStrategy
conflictResolutionStrategy) {
- lockConfig.setValue(WRITE_CONFLICT_RESOLUTION_STRATEGY_CLASS_NAME,
conflictResolutionStrategy.getClass().getName());
+ return
withConflictResolutionStrategyClassName(conflictResolutionStrategy.getClass().getName());
+ }
+
+ public HoodieLockConfig.Builder
withConflictResolutionStrategyClassName(String
conflictResolutionStrategyClassName) {
Review Comment:
nit: could we avoid the new setter method in the builder class?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3589,9 +3627,10 @@ private void validate() {
writeConcurrencyMode.name()));
}
if (writeConcurrencyMode ==
WriteConcurrencyMode.NON_BLOCKING_CONCURRENCY_CONTROL) {
+ boolean isMetadataTable =
HoodieTableMetadata.isMetadataTable(writeConfig.getBasePath());
checkArgument(
- writeConfig.getTableType().equals(HoodieTableType.MERGE_ON_READ)
&& writeConfig.isSimpleBucketIndex(),
- "Non-blocking concurrency control requires the MOR table with
simple bucket index");
+ writeConfig.getTableType().equals(HoodieTableType.MERGE_ON_READ)
&& (isMetadataTable || writeConfig.isSimpleBucketIndex()),
+ "Non-blocking concurrency control requires the MOR table with
simple bucket index or it has to be Metadata table");
Review Comment:
The reason I asked is, if the current concurrency control for MDT is
different from NBCC, might be better to have a new concurrency control mode for
MDT to avoid confusion and errors because of changing NBCC for data table in
general.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/MetadataTableNonBlockingWritesConflictResolutionStrategy.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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;
+
+import org.apache.hudi.common.model.HoodieCommitMetadata;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.exception.HoodieMetadataException;
+import org.apache.hudi.exception.HoodieWriteConflictException;
+import org.apache.hudi.table.HoodieTable;
+
+import java.util.stream.Stream;
+
+/**
+ * Conflict resolution strategy to be used with metadata table when NBCC is
enabled.
+ * This resolution will not conflict any operations and let everything succeed
the conflict resolution step.
+ */
+public class MetadataTableNonBlockingWritesConflictResolutionStrategy
implements ConflictResolutionStrategy {
+
+ @Override
+ public Stream<HoodieInstant> getCandidateInstants(HoodieTableMetaClient
metaClient, HoodieInstant currentInstant, Option<HoodieInstant>
lastSuccessfulInstant) {
+ return Stream.empty();
Review Comment:
As safety guard, add a validation on the `metaClient` to make sure it is a
metadata table, to avoid misuse.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -2855,8 +2864,15 @@ public Integer getWritesFileIdEncoding() {
return props.getInteger(WRITES_FILEID_ENCODING,
HoodieMetadataPayload.RECORD_INDEX_FIELD_FILEID_ENCODING_UUID);
}
- public boolean needResolveWriteConflict(WriteOperationType operationType) {
+ public boolean needResolveWriteConflict(WriteOperationType operationType,
boolean isMetadataTable, HoodieWriteConfig config,
+ HoodieTableConfig tableConfig) {
WriteConcurrencyMode mode = getWriteConcurrencyMode();
+ if (isMetadataTable &&
config.isStreamingWritesToMetadataEnabled(tableConfig.getTableVersion())) {
+ // for metadata table, if streaming writes are enabled, we might need to
perform conflict resolution.
+ // datatable NBCC is still evolving and might go through evolution
compared to its current state.
+ // But incase of metadata table, when streaming writes are enabled, we
are just looking to let every commit succeed w/o any resolution as such.
Review Comment:
nit: add a statement on
`MetadataTableNonBlockingWritesConflictResolutionStrategy` which is used to
bypass the conflict resolution by design, and opens up future capabilities on
conflict resolution in MDT
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3589,9 +3627,10 @@ private void validate() {
writeConcurrencyMode.name()));
}
if (writeConcurrencyMode ==
WriteConcurrencyMode.NON_BLOCKING_CONCURRENCY_CONTROL) {
+ boolean isMetadataTable =
HoodieTableMetadata.isMetadataTable(writeConfig.getBasePath());
checkArgument(
- writeConfig.getTableType().equals(HoodieTableType.MERGE_ON_READ)
&& writeConfig.isSimpleBucketIndex(),
- "Non-blocking concurrency control requires the MOR table with
simple bucket index");
+ writeConfig.getTableType().equals(HoodieTableType.MERGE_ON_READ)
&& (isMetadataTable || writeConfig.isSimpleBucketIndex()),
+ "Non-blocking concurrency control requires the MOR table with
simple bucket index or it has to be Metadata table");
Review Comment:
nit: L2870 has check on MDT already so do we still need to set NBCC for MDT
here or is `NON_BLOCKING_CONCURRENCY_CONTROL` is still required to be set so
the logic other than the conflict resolution strategy needs to be invoked?
--
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]
