vinothchandar commented on code in PR #11866:
URL: https://github.com/apache/hudi/pull/11866#discussion_r1762213770
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -942,6 +959,10 @@ protected void rollbackFailedWrites(Map<String,
Option<HoodiePendingRollbackInfo
}
protected void rollbackFailedWrites(Map<String,
Option<HoodiePendingRollbackInfo>> instantsToRollback, boolean skipLocking) {
+ rollbackFailedWrites(instantsToRollback, skipLocking, false);
+ }
+
+ protected void rollbackFailedWrites(Map<String,
Option<HoodiePendingRollbackInfo>> instantsToRollback, boolean skipLocking,
boolean skipVersionCheck) {
Review Comment:
need to create new method for handling two cases .
1. rollback outside of migration path (should check version)
2.rollback done pre upgrade/downgrade/migration (should be allowed to skip)
This is an open issue, we can come back to after making rollback table
version aware.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -881,6 +909,10 @@ public String startCommit() {
* Provides a new commit time for a write operation
(insert/update/delete/insert_overwrite/insert_overwrite_table) with specified
action.
*/
public String startCommit(String actionType, HoodieTableMetaClient
metaClient) {
+ if (needsUpgradeOrDowngrade(metaClient)) {
Review Comment:
the `startCommitXXX` methods don't create table upfront, instead use service
client to first rollback, which fails without an upgrade done first.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/table/TestHoodieTableConfig.java:
##########
@@ -236,4 +241,43 @@ public void testPartitionFieldAPIs(String partitionFields)
{
assertArrayEquals(new String[] {"p1", "p2"},
HoodieTableConfig.getPartitionFields(config).get());
assertEquals("p1",
HoodieTableConfig.getPartitionFieldWithoutKeyGenPartitionType(partitionFields.split(",")[0],
config));
}
+
+ @Test
+ public void testValidateConfigVersion() {
Review Comment:
table config validation tests
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/UpgradeDowngrade.java:
##########
@@ -62,10 +62,16 @@ public UpgradeDowngrade(
this.upgradeDowngradeHelper = upgradeDowngradeHelper;
}
- public boolean needsUpgradeOrDowngrade(HoodieTableVersion toVersion) {
- HoodieTableVersion fromVersion =
metaClient.getTableConfig().getTableVersion();
- // Ensure versions are same
- return toVersion.versionCode() != fromVersion.versionCode();
+ public boolean needsUpgradeOrDowngrade(HoodieTableVersion toWriteVersion) {
+ HoodieTableVersion fromTableVersion =
metaClient.getTableConfig().getTableVersion();
+
+ if (!config.autoUpgrade() && fromTableVersion.versionCode() <
toWriteVersion.versionCode()) {
Review Comment:
auto upgrade is on by default (current behavior). when its not, and versions
mismatch for an upgrade, we error out.
##########
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTableCommand.java:
##########
@@ -158,13 +159,14 @@ public void testDefaultCreate() {
public void testCreateWithSpecifiedValues() {
// Test create with specified values
Object result = shell.evaluate(() -> "create --path " + tablePath + "
--tableName " + tableName
- + " --tableType MERGE_ON_READ --archiveLogFolder archive");
+ + " --tableType MERGE_ON_READ --archiveLogFolder archive
--tableVersion 6");
Review Comment:
test covering CLI table creation
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableVersion.java:
##########
@@ -18,38 +18,43 @@
package org.apache.hudi.common.table;
+import org.apache.hudi.common.util.CollectionUtils;
import org.apache.hudi.exception.HoodieException;
import java.util.Arrays;
+import java.util.List;
/**
* Table's version that controls what version of writer/readers can actually
read/write
* to a given table.
*/
public enum HoodieTableVersion {
// < 0.6.0 versions
- ZERO(0),
+ ZERO(0, CollectionUtils.createImmutableList("0.3.0")),
Review Comment:
mapping major versions to table versions directly.. used in validation
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/upgrade/TestUpgradeDowngrade.java:
##########
@@ -171,6 +172,17 @@ public void cleanUp() throws Exception {
cleanupResources();
}
+ @Test
+ public void testAutoUpgradeFailure() throws IOException {
Review Comment:
new test to failing without auto upgrade
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1295,6 +1337,42 @@ public final HoodieTable initTable(WriteOperationType
operationType, Option<Stri
return table;
}
+ public void validateAgainstTableProperties(HoodieTableConfig tableConfig,
HoodieWriteConfig writeConfig) {
Review Comment:
this has been moved from HoodieTableMetaClient.. this was only called here.
and we should not be validating write configs at that layer.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -320,6 +327,27 @@ private void saveInternalSchema(HoodieTable table, String
instantTime, HoodieCom
}
}
+ protected HoodieTable createTableAndValidate(HoodieWriteConfig writeConfig,
Review Comment:
similar all table write operations call this to validate version, before
beginning any operation.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -675,7 +677,22 @@ protected Option<String>
scheduleTableServiceInternal(String instantTime, Option
return option;
}
- protected abstract HoodieTable<?, I, ?, T> createTable(HoodieWriteConfig
config, StorageConfiguration<?> storageConf);
+ protected HoodieTable createTableAndValidate(HoodieWriteConfig config,
Review Comment:
method that centralizes table version validation. This method is called from
all table services, to obtain `HoodieTable` before scheduling/planning.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -331,6 +342,23 @@ public class HoodieTableConfig extends HoodieConfig {
private static final String TABLE_CHECKSUM_FORMAT = "%s.%s"; //
<database_name>.<table_name>
+ static List<ConfigProperty<?>> definedTableConfigs() {
Review Comment:
this provides all declared config properties here. in the file.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -671,32 +742,6 @@ public String getPartitionFieldProp() {
return getPartitionFieldProp(this).orElse("");
}
- /**
- * Read the payload class for HoodieRecords from the table properties.
- */
- public String getBootstrapIndexClass() {
Review Comment:
unused methods.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestMultipleTableVersionWriting.scala:
##########
@@ -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.hudi
+
+import org.apache.hudi.common.model.HoodieTableType
+import org.apache.hudi.common.table.{HoodieTableConfig, HoodieTableMetaClient,
HoodieTableVersion}
+import org.apache.hudi.common.testutils.HoodieTestUtils
+import org.apache.hudi.config.HoodieWriteConfig
+import org.apache.hudi.exception.HoodieNotSupportedException
+import org.apache.spark.sql.SaveMode
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
+import org.scalatest.Assertions.assertThrows
+
+import java.util.Properties
+
+class TestMultipleTableVersionWriting extends HoodieSparkWriterTestBase {
Review Comment:
test at the spark datasource layer, since there is all the row vs non row
writer paths
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -120,12 +125,14 @@ public class HoodieTableConfig extends HoodieConfig {
+ "breaking/backwards compatible changes.");
public static final ConfigProperty<HoodieTableVersion> INITIAL_VERSION =
ConfigProperty
- .key("hoodie.table.initial.version")
- .defaultValue(HoodieTableVersion.ZERO)
- .withDocumentation("Initial Version of table when the table was
created. Used for upgrade/downgrade"
- + " to identify what upgrade/downgrade paths happened on the
table. This is only configured "
- + "when the table is initially setup.");
+ .key("hoodie.table.initial.version")
+ .defaultValue(HoodieTableVersion.current())
+ .sinceVersion("1.0.0")
Review Comment:
this was missing.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -528,68 +529,10 @@ private HoodieArchivedTimeline
instantiateArchivedTimeline(String startTs) {
: new HoodieArchivedTimeline(this, startTs);
}
- /**
- * Validate table properties.
- *
- * @param properties Properties from writeConfig.
- */
- public void validateTableProperties(Properties properties) {
- // Once meta fields are disabled, it cant be re-enabled for a given table.
- if (!getTableConfig().populateMetaFields()
- && Boolean.parseBoolean((String)
properties.getOrDefault(HoodieTableConfig.POPULATE_META_FIELDS.key(),
HoodieTableConfig.POPULATE_META_FIELDS.defaultValue().toString()))) {
- throw new HoodieException(HoodieTableConfig.POPULATE_META_FIELDS.key() +
" already disabled for the table. Can't be re-enabled back");
- }
-
- // Meta fields can be disabled only when either {@code
SimpleKeyGenerator}, {@code ComplexKeyGenerator}, {@code
NonpartitionedKeyGenerator} is used
- if (!getTableConfig().populateMetaFields()) {
- String keyGenClass = KeyGeneratorType.getKeyGeneratorClassName(new
HoodieConfig(properties));
- if (StringUtils.isNullOrEmpty(keyGenClass)) {
- keyGenClass = "org.apache.hudi.keygen.SimpleKeyGenerator";
- }
- if (!keyGenClass.equals("org.apache.hudi.keygen.SimpleKeyGenerator")
- &&
!keyGenClass.equals("org.apache.hudi.keygen.NonpartitionedKeyGenerator")
- &&
!keyGenClass.equals("org.apache.hudi.keygen.ComplexKeyGenerator")) {
- throw new HoodieException("Only simple, non-partitioned or complex key
generator are supported when meta-fields are disabled. Used: " + keyGenClass);
- }
- }
-
- //Check to make sure it's not a COW table with consistent hashing bucket
index
- if (tableType == HoodieTableType.COPY_ON_WRITE) {
- String indexType = properties.getProperty("hoodie.index.type");
- if (indexType != null && indexType.equals("BUCKET")) {
- String bucketEngine =
properties.getProperty("hoodie.index.bucket.engine");
- if (bucketEngine != null && bucketEngine.equals("CONSISTENT_HASHING"))
{
- throw new HoodieException("Consistent hashing bucket index does not
work with COW table. Use simple bucket index or an MOR table.");
- }
- }
- }
- }
-
- /**
- * Helper method to initialize a given path as a hoodie table with configs
passed in as Properties.
- *
- * @return Instance of HoodieTableMetaClient
- */
- public static HoodieTableMetaClient
initTableAndGetMetaClient(StorageConfiguration<?> storageConf, String basePath,
Review Comment:
all of these methods are cleaned. up. just two methods `initTable()`. in
TableBuilder (used for bulding/creating the table).. Builder class is used for
reading.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -470,6 +497,8 @@ public static void delete(HoodieStorage storage,
StoragePath metadataFolder, Set
/**
* Initialize the hoodie meta directory and any necessary files inside the
meta (including the hoodie.properties).
+ *
+ * TODO: this directory creation etc should happen in the
HoodieTableMetaClient.
Review Comment:
prefer to leave these in, for later.
--
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]
