[GitHub] [hudi] vinothchandar commented on a change in pull request #1100: [HUDI-289] Implement a test suite to support long running test for Hudi writing and querying end-end
vinothchandar commented on a change in pull request #1100: URL: https://github.com/apache/hudi/pull/1100#discussion_r457851643 ## File path: hudi-hadoop-mr/pom.xml ## @@ -125,6 +125,10 @@ mockito-junit-jupiter test + + org.mockito + mockito-junit-jupiter Review comment: yes.. lets remove this.. may be a rebase/merge thing? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hudi] vinothchandar commented on a change in pull request #1100: [HUDI-289] Implement a test suite to support long running test for Hudi writing and querying end-end
vinothchandar commented on a change in pull request #1100: URL: https://github.com/apache/hudi/pull/1100#discussion_r435662207 ## File path: hudi-spark/src/main/java/org/apache/hudi/keygen/NonpartitionedKeyGenerator.java ## @@ -38,9 +38,10 @@ public NonpartitionedKeyGenerator(TypedProperties props) { @Override public HoodieKey getKey(GenericRecord record) { -String recordKey = DataSourceUtils.getNestedFieldValAsString(record, recordKeyField, true); +String recordKey = DataSourceUtils.getNestedFieldValAsString(record, recordKeyFields.get(0), true); Review comment: Ideally we should make KeyGenerator an interface.. we have it as a class so we can force a certain constructor.. so again, this class should be free to implement by taking the single key field etc ## File path: pom.xml ## @@ -549,7 +551,52 @@ 5.17.2 - + Review comment: Why do we need these deps? Conceptually our integ-test Is already working with existing bundles and existing dependencies.. we should not need any changes to add the test-suite ## File path: hudi-spark/src/main/scala/org/apache/hudi/DataSourceOptions.scala ## @@ -265,6 +265,7 @@ object DataSourceWriteOptions { val HIVE_ASSUME_DATE_PARTITION_OPT_KEY = "hoodie.datasource.hive_sync.assume_date_partitioning" val HIVE_USE_PRE_APACHE_INPUT_FORMAT_OPT_KEY = "hoodie.datasource.hive_sync.use_pre_apache_input_format" val HIVE_USE_JDBC_OPT_KEY = "hoodie.datasource.hive_sync.use_jdbc" + val HIVE_ENABLE_TEST_SUITE_OPT_KEY = "hoodie.datasource.hive_sync.run_test_suite" Review comment: Please rename/remove ## File path: hudi-test-suite/src/main/java/org/apache/hudi/testsuite/DeltaWriteClient.java ## @@ -0,0 +1,219 @@ +/* + * 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.testsuite; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import org.apache.hadoop.conf.Configuration; +import org.apache.hudi.avro.model.HoodieCompactionPlan; +import org.apache.hudi.client.HoodieReadClient; +import org.apache.hudi.client.HoodieWriteClient; +import org.apache.hudi.client.WriteStatus; +import org.apache.hudi.common.model.HoodieRecord; +import org.apache.hudi.common.table.timeline.HoodieActiveTimeline; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.common.util.collection.Pair; +import org.apache.hudi.config.HoodieCompactionConfig; +import org.apache.hudi.config.HoodieIndexConfig; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.index.HoodieIndex; +import org.apache.hudi.testsuite.HoodieTestSuiteJob.HoodieTestSuiteConfig; +import org.apache.hudi.testsuite.dag.nodes.CleanNode; +import org.apache.hudi.testsuite.dag.nodes.DagNode; +import org.apache.hudi.testsuite.dag.nodes.RollbackNode; +import org.apache.hudi.testsuite.dag.nodes.ScheduleCompactNode; +import org.apache.hudi.utilities.deltastreamer.HoodieDeltaStreamer.Operation; +import org.apache.hudi.utilities.schema.SchemaProvider; +import org.apache.spark.api.java.JavaRDD; +import org.apache.spark.api.java.JavaSparkContext; + +/** + * A writer abstraction for the Hudi test suite. This class wraps different implementations of writers used to perform + * write operations into the target hudi dataset. Current supported writers are {@link HoodieDeltaStreamerWrapper} + * and {@link HoodieWriteClient}. + */ +public class DeltaWriteClient { Review comment: Rename to HudiTestSuiteDriver or anything else like that? ## File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/SourceFormatAdapter.java ## @@ -71,11 +71,11 @@ public SourceFormatAdapter(Source source) { // pass in the schema for the Row-to-Avro conversion // to avoid nullability mismatch between Avro schema and Row schema ? AvroConversionUtils.createRdd( -rdd, r.getSchemaProvider().getSourceSchema(), -HOODIE_RECORD_STRUCT_NAME,
[GitHub] [hudi] vinothchandar commented on a change in pull request #1100: [HUDI-289] Implement a test suite to support long running test for Hudi writing and querying end-end
vinothchandar commented on a change in pull request #1100: URL: https://github.com/apache/hudi/pull/1100#discussion_r435640221 ## File path: docker/demo/config/test-suite/test-source.properties ## @@ -0,0 +1,12 @@ +hoodie.datasource.write.recordkey.field=_row_key Review comment: Same question on license (can’t recall if we do this for existing property files or not? ## File path: docker/demo/config/test-suite/target.avsc ## @@ -0,0 +1,37 @@ +{ Review comment: I feel this schema exists is so many places :) ## File path: docker/demo/config/test-suite/complex-dag-cow.yaml ## @@ -0,0 +1,82 @@ +first_insert: Review comment: Can these take a license to? Check with other Apache projects? ## File path: hudi-hive-sync/src/test/java/org/apache/hudi/hive/testutils/HiveTestService.java ## @@ -90,7 +89,7 @@ public HiveServer2 start() throws IOException { Objects.requireNonNull(workDir, "The work dir must be set before starting cluster."); if (hadoopConf == null) { - hadoopConf = HoodieTestUtils.getDefaultHadoopConf(); + hadoopConf = new Configuration(); Review comment: Why does this have to change ## File path: hudi-spark/src/main/java/org/apache/hudi/DataSourceUtils.java ## @@ -293,6 +293,8 @@ public static HiveSyncConfig buildHiveSyncConfig(TypedProperties props, String b SlashEncodedDayPartitionValueExtractor.class.getName()); hiveSyncConfig.useJdbc = Boolean.valueOf(props.getString(DataSourceWriteOptions.HIVE_USE_JDBC_OPT_KEY(), DataSourceWriteOptions.DEFAULT_HIVE_USE_JDBC_OPT_VAL())); +hiveSyncConfig.isTestSuite = props.getBoolean(DataSourceWriteOptions.HIVE_ENABLE_TEST_SUITE_OPT_KEY(), Review comment: Again, we cannot leak the test suite stuff into real code ## File path: hudi-hadoop-mr/pom.xml ## @@ -125,6 +125,10 @@ mockito-junit-jupiter test + + org.mockito + mockito-junit-jupiter Review comment: Scope: test? Or is that inherited ## File path: hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfig.java ## @@ -71,6 +71,9 @@ @Parameter(names = {"--skip-ro-suffix"}, description = "Skip the `_ro` suffix for Read optimized table, when registering") public Boolean skipROSuffix = false; + @Parameter(names = {"--is-test-suite"}, description = "Hudi test suite") Review comment: Let’s rename this to reflect what functionally it affects.. this does not look very nice ## File path: hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java ## @@ -108,6 +108,12 @@ private void syncHoodieTable(String tableName, boolean useRealtimeInputFormat) { + " of type " + hoodieHiveClient.getTableType()); // Check if the necessary table exists boolean tableExists = hoodieHiveClient.doesTableExist(tableName); + +// check if the database exists else create it +if (cfg.isTestSuite) { + LOG.info("Test suite specific creation of DB " + cfg.isTestSuite); Review comment: Change this log statement well ## File path: hudi-spark/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java ## @@ -98,12 +94,4 @@ public HoodieKey getKey(GenericRecord record) { return new HoodieKey(recordKey.toString(), partitionPath.toString()); } - - public List getRecordKeyFields() { Review comment: Unused methods? Some annotation of the PR to reason about the changes would be great ## File path: hudi-spark/src/main/java/org/apache/hudi/QuickstartUtils.java ## @@ -48,6 +48,30 @@ */ public class QuickstartUtils { + private static Option convertToString(HoodieRecord record) { +try { + String str = HoodieAvroUtils + .bytesToAvro(((OverwriteWithLatestAvroPayload) record.getData()).recordBytes, DataGenerator.avroSchema) + .toString(); + str = "{" + str.substring(str.indexOf("\"ts\":")); + return Option.of(str.replaceAll("}", ", \"partitionpath\": \"" + record.getPartitionPath() + "\"}")); +} catch (IOException e) { + return Option.empty(); +} + } + + public static List convertToStringList(List records) { Review comment: Are you just moving code around. If so why? ## File path: hudi-spark/src/main/java/org/apache/hudi/QuickstartUtils.java ## @@ -48,6 +48,30 @@ */ public class QuickstartUtils { + private static Option convertToString(HoodieRecord record) { +try { + String str = HoodieAvroUtils + .bytesToAvro(((OverwriteWithLatestAvroPayload) record.getData()).recordBytes, DataGenerator.avroSchema) + .toString(); + str = "{" + str.substring(str.indexOf("\"ts\":")); + return Option.of(str.replaceAll("}", ", \"partitionpath\": \"" + record.getPartitionPath() + "\"}")); +} catch (IOException e) { + return Option.empty(); +