the-other-tim-brown commented on code in PR #13236: URL: https://github.com/apache/hudi/pull/13236#discussion_r2077890954
########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/LeanWriteStatus.java: ########## @@ -0,0 +1,47 @@ +/* + * 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; + +/** + * Lean version of {@link WriteStatus} where all additional stats like rli, col sats etc are trimmed off. + */ +public class LeanWriteStatus extends WriteStatus { Review Comment: Some thoughts here: 1. Does it make more sense to have the WriteStatus extend LeanWriteStatus? 2. Is there another name we should use to make it more clear when to use one class as compared to other? The WriteStatus is now containing more metadata whereas the LeanStatus is meant to have the actual final status of the write. If we go this route, you can also make the data required for the final result a member of the object that has the metadata as well. This `has a` instead of `is a` style relationship may make more sense. ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieClient.java: ########## @@ -262,6 +282,18 @@ protected void finalizeWrite(HoodieTable table, String instantTime, List<HoodieW } } + class GetMetadataWriterFunc implements Functions.Function2<String, HoodieTableMetaClient, Option<HoodieTableMetadataWriter>> { Review Comment: You can also define a FunctionalInterface to make it more explicit rather than relying on the general `Functions` ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieClient.java: ########## @@ -262,6 +282,18 @@ protected void finalizeWrite(HoodieTable table, String instantTime, List<HoodieW } } + class GetMetadataWriterFunc implements Functions.Function2<String, HoodieTableMetaClient, Option<HoodieTableMetadataWriter>> { + + @Override + public Option<HoodieTableMetadataWriter> apply(String triggeringInstantTimestamp, HoodieTableMetaClient metaClient) { + return getMetadataWriter(triggeringInstantTimestamp, metaClient); + } + } + + Option<HoodieTableMetadataWriter> getMetadataWriter(String triggeringInstantTimestamp, HoodieTableMetaClient metaClient) { + throw new HoodieException("Each engine's write client is expected to implement this method"); Review Comment: You can just make this `protected abstract` to require this right? ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java: ########## @@ -369,6 +448,68 @@ protected void completeCompaction(HoodieCommitMetadata metadata, HoodieTable tab LOG.info("Compacted successfully on commit {}", compactionCommitTime); } + public void commitLogCompaction(String compactionInstantTime, HoodieWriteMetadata<O> compactionWriteMetadata, Option<HoodieTable> tableOpt, Review Comment: Does this need to be public? Let's make sure there are javadocs as well since the `completeLogCompaction` can also be used for commits based on the comments so it is not clear when a developer would want to call one or the other ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/WriteStatus.java: ########## @@ -76,10 +78,15 @@ public class WriteStatus implements Serializable { private final boolean trackSuccessRecords; private final transient Random random; - public WriteStatus(Boolean trackSuccessRecords, Double failureFraction) { + public WriteStatus(Boolean trackSuccessRecords, Double failureFraction, Boolean isMetadataTable) { Review Comment: If `Boolean` is required be sure to handle the `null` case so you don't end up with an NPE at runtime ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java: ########## @@ -221,8 +234,25 @@ public boolean commitStats(String instantTime, List<HoodieWriteStat> stats, Option<Map<String, String>> extraMetadata, String commitActionType, Map<String, List<String>> partitionToReplaceFileIds, Option<BiConsumer<HoodieTableMetaClient, HoodieCommitMetadata>> extraPreCommitFunc) { + return commitStats(instantTime, stats, Collections.emptyList(), extraMetadata, commitActionType, + partitionToReplaceFileIds, extraPreCommitFunc); + } + + public boolean commitStats(String instantTime, List<HoodieWriteStat> dataTableStats, List<HoodieWriteStat> mdtStats, Review Comment: Let's also add javadocs here and make sure you're intentional about the scope of the methods so we can limit the points another developer can interact with to the smallest surface area. It will make it easier for us to maintain the code in the future ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java: ########## @@ -1427,7 +1442,11 @@ public boolean isConsistentLogicalTimestampEnabled() { } public Boolean shouldAutoCommit() { - return getBoolean(AUTO_COMMIT_ENABLE); + return false; + } + + public Boolean shouldInternalAutoCommit() { Review Comment: Can we make this return a primitive boolean? -- 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]
