[HUDI-3172] Refactor hudi existing modules to make more code reuse in V2 Implementation by leesf · Pull Request #4514 · apache/hudi

leesf · 2022-01-05T11:13:49Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

Introduce hudi-spark3-common and hudi-spark2-common modules to place classes that would be reused in different spark versions, also introduce hudi-spark3.1.x to support spark 3.1.x
Introduce hudi format under hudi-spark2, hudi-spark3, hudi-spark3.1.x modules and change the hudi format in original hudi-spark module to hudi_v1 format
Manually tested on Spark 3.1.2 and Spark 3.2.0 SQL.

Brief change log

(for example:)

Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

leesf · 2022-01-05T13:39:06Z

@hudi-bot run azure

vinothchandar

Let me make another pass at all the pom changes. That seems to be main thing here.
In the meantime, could you clarify these comments?

Also have you tested these changes across spark 2.x and 3.1/3.2 bundles ?

xushiyan · 2022-01-06T06:17:04Z

  }

-  override def shortName(): String = "hudi"
+  override def shortName(): String = "hudi_v1"


@leesf i suppose this refactoring PR not meant to include this change?

If not change the format, it would conflict with hudi-spark2/hudi-spark3.1.x/hudi-spark3 module format.

would it conflict? Given we are extending DefaultSource and overriding shortName()?

it is because in hudi-spark-bundle module. I used <transformer implementation="org.apache.maven.plugins.shade.resource.AppendingTransformer"> <resource>META-INF/services/org.apache.spark.sql.sources.DataSourceRegister</resource> </transformer> to append the formats(hudi_v1 and hudi) in DataSourceRegister file, so it will conflict if not change the format. As to the PR itself, we do not need to change the format to hudi_v1 and not use AppendingTransformer. But when implementing V2 codepath, I find it difficult to handle the incremental bootstrap table(

hudi/hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/IncrementalRelation.scala

Line 175 in ac8d014

.schema(usedSchema)

) as it will pass schema to Spark and difficult to handle in v2 codepath, after finding a good way to handle it, we would definitely delete the hudi_v1 format here.

leesf · 2022-01-06T08:14:25Z

Let me make another pass at all the pom changes. That seems to be main thing here. In the meantime, could you clarify these comments?

Also have you tested these changes across spark 2.x and 3.1/3.2 bundles ?

@vinothchandar Yes, I have manually tested it with spark 3.2.0 and spark 3.1.2 version on spark sql. and the CI tested it on spark 2.4.x and works well.

xushiyan

LGTM. Since a lot of files were moved, cherry pick later commits for 0.10.1 could be problematic. Shall we move this into a feature branch?

vinothchandar · 2022-01-07T03:54:19Z

@xushiyan given we are almost winding down for 0.10.1, I suggest we land this sooner than later. That way we can focus on stabilizing master for 0.11.0 - that's not too far away. wdyt?

xushiyan · 2022-01-07T05:35:32Z

@xushiyan given we are almost winding down for 0.10.1, I suggest we land this sooner than later. That way we can focus on stabilizing master for 0.11.0 - that's not too far away. wdyt?

it won't be too far away. @nsivabalan is cherry picking for 0.10.1 which will complete by Jan 9. Holding this off for 2 more days can avoid conflicts from some Spark fixes merged after this. Seeing 2 more fixes coming. After that we should be able to land this right away.

vinothchandar · 2022-01-07T05:55:44Z

2 more days should be okay?

vinothchandar · 2022-01-07T06:26:40Z

@leesf few questions.

What do the hudi-spark2-extensions/hudi-spark3-extensions do? What code would these have in the future?
Users may have spark jobs that depend directly on hudi-spark . any impact to these from the changes?
I feel there are way too many packages now - hudi-spark*, then it depends hudi-spark-common* . Is there a way for us to merge hudi-spark-common into hudi-spark* ? is the hudi-spark3-common so we can share code across the different 3.x versions? Can you add a README that explains what code should go where going forward
Any updates to release notes or README?

vinothchandar

Made some cleanup suggestions. LGTM overall

nsivabalan · 2022-01-07T12:44:34Z

yeah, would really appreciate if we can wait until Jan 9 to land this patch. thanks!

leesf · 2022-01-07T14:18:56Z

@leesf few questions.

What do the hudi-spark2-extensions/hudi-spark3-extensions do? What code would these have in the future?

Users may have spark jobs that depend directly on hudi-spark . any impact to these from the changes?

I feel there are way too many packages now - hudi-spark*, then it depends hudi-spark-common* . Is there a way for us to merge hudi-spark-common into hudi-spark* ? is the hudi-spark3-common so we can share code across the different 3.x versions? Can you add a README that explains what code should go where going forward

Any updates to release notes or README?

I did intend to remove the modules but wrongly added. removed now.
I did not change hudi-spark module, so it should have no impact on the users.
I think the current modules has be in a good shape, if merge code from hudi-spark-common into hudi-spark* or merge code from hudi-spark3-common into hudi-spark3 & spark3.1.x, there would be many duplicate classes, which i think is not an ideal way.
Added a README under hudi-spark-datasource module.

leesf · 2022-01-11T01:35:26Z

yeah, would really appreciate if we can wait until Jan 9 to land this patch. thanks!

@nsivabalan @xushiyan time to land this patch?

xushiyan · 2022-01-11T05:33:31Z

@leesf yes we won't have conflicting patches to pick from master. we can land this one now.

leesf · 2022-01-11T07:56:33Z

@leesf yes we won't have conflicting patches to pick from master. we can land this one now.

@xushiyan CI passed, so can we merge?

… V2 implementation

leesf · 2022-01-13T07:37:21Z

@hudi-bot run azure

hudi-bot · 2022-01-13T08:31:59Z

CI report:

ddc3af0 UNKNOWN
e1ba726 UNKNOWN
306e7d4 UNKNOWN
1512277 UNKNOWN
0a64e41 UNKNOWN
4fbef93 Azure: FAILURE Azure: FAILURE Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

… V2 Implementation (apache#4514) * Introduce hudi-spark3-common and hudi-spark2-common modules to place classes that would be reused in different spark versions, also introduce hudi-spark3.1.x to support spark 3.1.x. * Introduce hudi format under hudi-spark2, hudi-spark3, hudi-spark3.1.x modules and change the hudi format in original hudi-spark module to hudi_v1 format. * Manually tested on Spark 3.1.2 and Spark 3.2.0 SQL. * Added a README.md file under hudi-spark-datasource module.

leesf force-pushed the HUDI-3172 branch 4 times, most recently from 306e7d4 to 3a20dc1 Compare January 5, 2022 12:01

leesf assigned xushiyan Jan 5, 2022

leesf requested a review from xushiyan January 5, 2022 12:01

leesf force-pushed the HUDI-3172 branch 3 times, most recently from d9381d0 to ac8d014 Compare January 6, 2022 05:15

vinothchandar changed the title ~~[HUDI-3172] Refactor hudi existing modules to make more code reuse in…~~ [HUDI-3172] Refactor hudi existing modules to make more code reuse in V2 Implementation Jan 6, 2022

vinothchandar reviewed Jan 6, 2022

View reviewed changes

xushiyan reviewed Jan 6, 2022

View reviewed changes

vinothchandar approved these changes Jan 7, 2022

View reviewed changes

leesf force-pushed the HUDI-3172 branch 3 times, most recently from b7b4aca to b6a98a3 Compare January 10, 2022 11:19

leesf force-pushed the HUDI-3172 branch from b6a98a3 to 3ca5b12 Compare January 11, 2022 06:14

leesf force-pushed the HUDI-3172 branch from 3ca5b12 to 9dfc12d Compare January 12, 2022 07:56

leesf added 4 commits January 13, 2022 12:05

[HUDI-3172] Refactor hudi existing modules to make more code reuse in…

de4dda7

… V2 implementation

fix comments

4642bb8

remove useless modules and add README

ba9269d

rebase to master and add missing dependencies

4fbef93

leesf force-pushed the HUDI-3172 branch from 9dfc12d to 4fbef93 Compare January 13, 2022 04:05

leesf merged commit 5ce45c4 into apache:master Jan 14, 2022

vinishjail97 mentioned this pull request Jan 24, 2022

FixIgnoreKey nsivabalan/hudi#11

Closed

5 tasks

Conversation

leesf commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

leesf commented Jan 5, 2022

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xushiyan Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

leesf Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

vinothchandar Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

leesf Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesf commented Jan 6, 2022

Uh oh!

xushiyan left a comment

Choose a reason for hiding this comment

Uh oh!

vinothchandar commented Jan 7, 2022

Uh oh!

xushiyan commented Jan 7, 2022

Uh oh!

vinothchandar commented Jan 7, 2022

Uh oh!

vinothchandar commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

nsivabalan commented Jan 7, 2022

Uh oh!

leesf commented Jan 7, 2022

Uh oh!

leesf commented Jan 11, 2022

Uh oh!

xushiyan commented Jan 11, 2022

Uh oh!

leesf commented Jan 11, 2022

Uh oh!

leesf commented Jan 13, 2022

Uh oh!

hudi-bot commented Jan 13, 2022

CI report:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leesf commented Jan 5, 2022 •

edited

Loading

leesf Jan 7, 2022 •

edited

Loading

vinothchandar commented Jan 7, 2022 •

edited

Loading