[GitHub] [hudi] alexeykudinkin commented on a diff in pull request #5523: [HUDI-4039] Make sure all builtin `KeyGenerator`s properly implement Spark specific APIs

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

alexeykudinkin commented on code in PR #5523:
URL: https://github.com/apache/hudi/pull/5523#discussion_r930232499


##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/storage/row/HoodieRowCreateHandle.java:
##########
@@ -137,47 +135,59 @@ public HoodieRowCreateHandle(HoodieTable table,
    * @throws IOException
    */
   public void write(InternalRow row) throws IOException {
+    if (populateMetaFields) {
+      writeRow(row);
+    } else {
+      writeRowNoMetaFields(row);
+    }
+  }
+
+  private void writeRow(InternalRow row) {
     try {
       // NOTE: PLEASE READ THIS CAREFULLY BEFORE MODIFYING
       //       This code lays in the hot-path, and substantial caution should be
       //       exercised making changes to it to minimize amount of excessive:
-      //          - Conversions b/w Spark internal (low-level) types and JVM native ones (like
-      //         [[UTF8String]] and [[String]])
+      //          - Conversions b/w Spark internal types and JVM native ones (like [[UTF8String]]
+      //          and [[String]])
       //          - Repeated computations (for ex, converting file-path to [[UTF8String]] over and
       //          over again)
-      UTF8String recordKey = row.getUTF8String(RECORD_KEY_META_FIELD_ORD);
-
-      InternalRow updatedRow;
-      // In cases when no meta-fields need to be added we simply relay provided row to
-      // the writer as is
-      if (!populateMetaFields) {
-        updatedRow = row;
-      } else {
-        UTF8String partitionPath = row.getUTF8String(PARTITION_PATH_META_FIELD_ORD);
-        // This is the only meta-field that is generated dynamically, hence conversion b/w
-        // [[String]] and [[UTF8String]] is unavoidable
-        UTF8String seqId = UTF8String.fromString(seqIdGenerator.apply(GLOBAL_SEQ_NO.getAndIncrement()));
-
-        updatedRow = new HoodieInternalRow(commitTime, seqId, recordKey,
-            partitionPath, fileName, row, true);
-      }
+      UTF8String recordKey = row.getUTF8String(HoodieRecord.RECORD_KEY_META_FIELD_ORD);
+      UTF8String partitionPath = row.getUTF8String(HoodieRecord.PARTITION_PATH_META_FIELD_ORD);
+      // This is the only meta-field that is generated dynamically, hence conversion b/w
+      // [[String]] and [[UTF8String]] is unavoidable
+      UTF8String seqId = UTF8String.fromString(seqIdGenerator.apply(GLOBAL_SEQ_NO.getAndIncrement()));
+
+      InternalRow updatedRow = new HoodieInternalRow(commitTime, seqId, recordKey,
+          partitionPath, fileName, row, true);
 
       try {
         fileWriter.writeRow(recordKey, updatedRow);
         // NOTE: To avoid conversion on the hot-path we only convert [[UTF8String]] into [[String]]
         //       in cases when successful records' writes are being tracked
         writeStatus.markSuccess(writeStatus.isTrackingSuccessfulWrites() ? recordKey.toString() : null);
-      } catch (Throwable t) {
+      } catch (Exception t) {

Review Comment:
   It's not gonna change it in the unexpected way: any application level are inheriting from `Exception`, the only things that do inherit from `Throwable` but not from `Exception` are the terminal ones (ie ones you can't recover from) like 
   `OutOfMemory`, etc



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org