You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2021/03/09 08:50:15 UTC

[GitHub] [hudi] sbernauer opened a new pull request #2650: Preparation for Avro update

sbernauer opened a new pull request #2650:
URL: https://github.com/apache/hudi/pull/2650


   Hi Hudi team!
   
   ## What is the purpose of the pull request
   We need to upgrade to at least Avro 1.9.x in production so i tried upgrading the avro version in the pom.xml of Hudi. Doing so i noticed some problems:
   
   Upgrade to Avro 1.9.2:
   - Renamed method defaultValue to defaultVal
   - Moved NullNode.getInstance() to JsonProperties.NULL_VALUE
   - Avro complains about invalid schemas/default values in hudi-common/src/main/avro/
   - The shaded guava libs from Avro have been removed
   
   Upgrade to Avro 1.10.1:
   - Some more stuff
   
   Spark 3.2.0 (we currently use 3.1.1) will contain Avro 1.10.1 (https://issues.apache.org/jira/browse/SPARK-27733).
   Ín order to reduce the effort switching to a newer Avro version in the future i provided a patches that fixes the above mentioned issues.
   
   ## Brief change log
   Fix the above mentioned issues
   
   ## Verify this pull request
   Run Unit tests
   
   Kind regards
   Sebastian
   


----------------------------------------------------------------
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

[GitHub] [hudi] sbernauer commented on a change in pull request #2650: Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
sbernauer commented on a change in pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#discussion_r594155813



##########
File path: hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/index/simple/FlinkHoodieSimpleIndex.java
##########
@@ -135,8 +133,8 @@ public boolean isImplicitWithStorage() {
         context.map(latestBaseFiles, partitionPathBaseFile -> new HoodieKeyLocationFetchHandle<>(config, hoodieTable, partitionPathBaseFile), parallelism);
     Map<HoodieKey, HoodieRecordLocation> recordLocations = new HashMap<>();
     hoodieKeyLocationFetchHandles.stream()
-        .flatMap(handle -> Lists.newArrayList(handle.locations()).stream())
-        .forEach(x -> x.forEach(y -> recordLocations.put(y.getKey(), y.getRight())));
+        .flatMap(handle -> handle.locations())

Review comment:
       This was changed because of the removal of the used guava libs ;)

##########
File path: hudi-common/src/main/avro/HoodieRestoreMetadata.avsc
##########
@@ -38,7 +38,6 @@
      /* overlaps with 'instantsToRollback' field. Adding this to track action type for all the instants being rolled back. */
      {
        "name": "restoreInstantInfo",
-       "default": null,

Review comment:
       The default value of null doesn't match top a field with type array. Instead of removing the default value of `null` i now changed it to an empty array `[]`




----------------------------------------------------------------
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

[GitHub] [hudi] n3nash merged pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
n3nash merged pull request #2650:
URL: https://github.com/apache/hudi/pull/2650


   


-- 
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

[GitHub] [hudi] danny0405 commented on a change in pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#discussion_r595940896



##########
File path: hudi-common/src/main/avro/HoodieRestoreMetadata.avsc
##########
@@ -38,7 +38,6 @@
      /* overlaps with 'instantsToRollback' field. Adding this to track action type for all the instants being rolled back. */
      {
        "name": "restoreInstantInfo",
-       "default": null,

Review comment:
       Yes, empty array `[]` has the correct element type.




----------------------------------------------------------------
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

[GitHub] [hudi] vinothchandar commented on a change in pull request #2650: Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#discussion_r593856693



##########
File path: hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/index/simple/FlinkHoodieSimpleIndex.java
##########
@@ -135,8 +133,8 @@ public boolean isImplicitWithStorage() {
         context.map(latestBaseFiles, partitionPathBaseFile -> new HoodieKeyLocationFetchHandle<>(config, hoodieTable, partitionPathBaseFile), parallelism);
     Map<HoodieKey, HoodieRecordLocation> recordLocations = new HashMap<>();
     hoodieKeyLocationFetchHandles.stream()
-        .flatMap(handle -> Lists.newArrayList(handle.locations()).stream())
-        .forEach(x -> x.forEach(y -> recordLocations.put(y.getKey(), y.getRight())));
+        .flatMap(handle -> handle.locations())

Review comment:
       seems orthgonal. but agree with the changes

##########
File path: hudi-common/src/main/avro/HoodieRestoreMetadata.avsc
##########
@@ -38,7 +38,6 @@
      /* overlaps with 'instantsToRollback' field. Adding this to track action type for all the instants being rolled back. */
      {
        "name": "restoreInstantInfo",
-       "default": null,

Review comment:
       could you help me understand why the default value was removed?

##########
File path: hudi-common/src/main/avro/HoodieClusteringPlan.avsc
##########
@@ -30,7 +30,7 @@
     },
     {
        "name":"strategy",
-       "type":["HoodieClusteringStrategy", "null"],
+       "type":["null", "HoodieClusteringStrategy"],

Review comment:
       oops. cc @satishkotha @n3nash  you may also want to look at this. default values work only based on the type of the first element in an union IIUC. 

##########
File path: hudi-common/src/main/avro/HoodieRequestedReplaceMetadata.avsc
##########
@@ -23,11 +23,11 @@
      {
          "name":"operationType",
          "type":["null", "string"],
-         "default": ""
+         "default": null

Review comment:
       same here
   




----------------------------------------------------------------
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

[GitHub] [hudi] codecov-io commented on pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#issuecomment-799292031


   # [Codecov](https://codecov.io/gh/apache/hudi/pull/2650?src=pr&el=h1) Report
   > Merging [#2650](https://codecov.io/gh/apache/hudi/pull/2650?src=pr&el=desc) (1a5cb70) into [master](https://codecov.io/gh/apache/hudi/commit/899ae70fdb70c1511c099a64230fd91b2fe8d4ee?el=desc) (899ae70) will **increase** coverage by `0.39%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/hudi/pull/2650/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/hudi/pull/2650?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2650      +/-   ##
   ============================================
   + Coverage     51.58%   51.97%   +0.39%     
   - Complexity     3285     3579     +294     
   ============================================
     Files           446      466      +20     
     Lines         20409    22275    +1866     
     Branches       2116     2374     +258     
   ============================================
   + Hits          10528    11578    +1050     
   - Misses         9003     9689     +686     
   - Partials        878     1008     +130     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | hudicli | `37.01% <ø> (+0.14%)` | `0.00 <ø> (ø)` | |
   | hudiclient | `100.00% <ø> (ø)` | `0.00 <ø> (ø)` | |
   | hudicommon | `51.46% <0.00%> (+0.04%)` | `0.00 <0.00> (ø)` | |
   | hudiflink | `53.57% <ø> (+2.28%)` | `0.00 <ø> (ø)` | |
   | hudihadoopmr | `33.44% <ø> (+0.28%)` | `0.00 <ø> (ø)` | |
   | hudisparkdatasource | `69.84% <ø> (+0.17%)` | `0.00 <ø> (ø)` | |
   | hudisync | `49.62% <ø> (ø)` | `0.00 <ø> (ø)` | |
   | huditimelineservice | `64.36% <ø> (ø)` | `0.00 <ø> (ø)` | |
   | hudiutilities | `69.48% <ø> (+0.04%)` | `0.00 <ø> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/hudi/pull/2650?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [.../apache/hudi/common/table/TableSchemaResolver.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL1RhYmxlU2NoZW1hUmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...pache/hudi/common/table/HoodieTableMetaClient.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL0hvb2RpZVRhYmxlTWV0YUNsaWVudC5qYXZh) | `68.31% <0.00%> (-2.46%)` | `43.00% <0.00%> (-1.00%)` | |
   | [...i/common/table/timeline/HoodieDefaultTimeline.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZURlZmF1bHRUaW1lbGluZS5qYXZh) | `82.66% <0.00%> (-2.27%)` | `59.00% <0.00%> (ø%)` | |
   | [...che/hudi/common/table/log/HoodieLogFileReader.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL2xvZy9Ib29kaWVMb2dGaWxlUmVhZGVyLmphdmE=) | `66.09% <0.00%> (-1.77%)` | `23.00% <0.00%> (+1.00%)` | :arrow_down: |
   | [.../hadoop/utils/HoodieRealtimeRecordReaderUtils.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL3V0aWxzL0hvb2RpZVJlYWx0aW1lUmVjb3JkUmVhZGVyVXRpbHMuamF2YQ==) | `71.79% <0.00%> (-1.25%)` | `30.00% <0.00%> (ø%)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `70.34% <0.00%> (-0.37%)` | `53.00% <0.00%> (+1.00%)` | :arrow_down: |
   | [...ies/sources/helpers/DatePartitionPathSelector.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvaGVscGVycy9EYXRlUGFydGl0aW9uUGF0aFNlbGVjdG9yLmphdmE=) | `54.68% <0.00%> (-0.16%)` | `13.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/hudi/sink/CommitSink.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1mbGluay9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVkaS9zaW5rL0NvbW1pdFNpbmsuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | [.../org/apache/hudi/util/RowDataToAvroConverters.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1mbGluay9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVkaS91dGlsL1Jvd0RhdGFUb0F2cm9Db252ZXJ0ZXJzLmphdmE=) | `42.05% <0.00%> (ø)` | `8.00% <0.00%> (ø%)` | |
   | [...che/hudi/common/table/timeline/HoodieTimeline.java](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZVRpbWVsaW5lLmphdmE=) | `91.30% <0.00%> (ø)` | `44.00% <0.00%> (ø%)` | |
   | ... and [74 more](https://codecov.io/gh/apache/hudi/pull/2650/diff?src=pr&el=tree-more) | |
   


----------------------------------------------------------------
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

[GitHub] [hudi] satishkotha commented on a change in pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
satishkotha commented on a change in pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#discussion_r596203816



##########
File path: hudi-common/src/main/avro/HoodieClusteringPlan.avsc
##########
@@ -30,7 +30,7 @@
     },
     {
        "name":"strategy",
-       "type":["HoodieClusteringStrategy", "null"],
+       "type":["null", "HoodieClusteringStrategy"],

Review comment:
       ack. thanks for fixes.




----------------------------------------------------------------
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

[GitHub] [hudi] vinothchandar commented on pull request #2650: Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#issuecomment-798851341


   cc @n3nash can you chime in here? 


----------------------------------------------------------------
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

[GitHub] [hudi] danny0405 commented on a change in pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#discussion_r595940896



##########
File path: hudi-common/src/main/avro/HoodieRestoreMetadata.avsc
##########
@@ -38,7 +38,6 @@
      /* overlaps with 'instantsToRollback' field. Adding this to track action type for all the instants being rolled back. */
      {
        "name": "restoreInstantInfo",
-       "default": null,

Review comment:
       Yes, empty has the correct element type.




----------------------------------------------------------------
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

[GitHub] [hudi] n3nash commented on pull request #2650: [HUDI-1694] Preparation for Avro update

Posted by GitBox <gi...@apache.org>.
n3nash commented on pull request #2650:
URL: https://github.com/apache/hudi/pull/2650#issuecomment-810758125


   This LGTM


-- 
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