[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();
+
