[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

[GitBox <gi...@apache.org>]

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 @@
       <artifactId>mockito-junit-jupiter</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-junit-jupiter</artifactId>

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<String> 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<String> 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<String> convertToStringList(List<HoodieRecord> 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<String> 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<String> convertToStringList(List<HoodieRecord> records) {

Review comment:
       If this is real code change, let’s run the QuickStart once and verify




----------------------------------------------------------------
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:
users@infra.apache.org