Skip to content

[HUDI-1328] Introduce HoodieFlinkEngineContext to hudi-flink-client#2161

Merged
leesf merged 1 commit intoapache:masterfrom
wangxianghu:HUDI-1328
Oct 14, 2020
Merged

[HUDI-1328] Introduce HoodieFlinkEngineContext to hudi-flink-client#2161
leesf merged 1 commit intoapache:masterfrom
wangxianghu:HUDI-1328

Conversation

@wangxianghu
Copy link
Contributor

Tips

What is the purpose of the pull request

Introduce HoodieFlinkEngineContext to hudi-flink-client

Brief change log

(for example:)

  • Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

This pull request is already covered by existing tests, such as TestHoodieFlinkEngineContext.

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.

@wangxianghu
Copy link
Contributor Author

@yanghua @leesf please take a look when free

@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #2161 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2161   +/-   ##
=========================================
  Coverage     53.61%   53.61%           
  Complexity     2845     2845           
=========================================
  Files           359      359           
  Lines         16535    16535           
  Branches       1777     1777           
=========================================
  Hits           8866     8866           
  Misses         6912     6912           
  Partials        757      757           
Flag Coverage Δ Complexity Δ
#hudicli 38.37% <ø> (ø) 193.00 <ø> (ø)
#hudiclient 100.00% <ø> (ø) 0.00 <ø> (ø)
#hudicommon 54.74% <ø> (ø) 1793.00 <ø> (ø)
#hudihadoopmr 33.05% <ø> (ø) 181.00 <ø> (ø)
#hudispark 65.51% <ø> (ø) 303.00 <ø> (ø)
#huditimelineservice 62.29% <ø> (ø) 50.00 <ø> (ø)
#hudiutilities 69.98% <ø> (ø) 325.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
.../org/apache/hudi/common/model/HoodieTableType.java 100.00% <0.00%> (ø) 1.00% <0.00%> (ø%)
...i/common/model/OverwriteWithLatestAvroPayload.java 64.70% <0.00%> (ø) 10.00% <0.00%> (ø%)
...del/OverwriteNonDefaultsWithLatestAvroPayload.java 78.94% <0.00%> (ø) 5.00% <0.00%> (ø%)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use HoodieException?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use HoodieException?

done

Comment on lines +74 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we would move the methods to super class as default implementation after finishing the flink integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we would move the methods to super class as default implementation after finishing the flink integration.

Good idea

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here would always get spark instead of flink?

Copy link
Contributor Author

@wangxianghu wangxianghu Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here would always get spark instead of flink?

It should be. List is ordered.
Assertions.assertNotNull(resultMap.get("hudi")) should be better understood

Copy link
Contributor

@leesf leesf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with the PR as-is. IIUC this change does not really add any flink related changes. It might be good to have PRs, such that add some functionality in each PR.

Also +1 on deferring further refactoring/reorganization until we can implement a first cut of flink support.

@wangxianghu
Copy link
Contributor Author

I am okay with the PR as-is. IIUC this change does not really add any flink related changes. It might be good to have PRs, such that add some functionality in each PR.

Also +1 on deferring further refactoring/reorganization until we can implement a first cut of flink support.

Yes, this PR actually aims to introduce hudi-flink-clientmodule, which is the basis of the incoming functionality PRs.
Agree with you that each PR should contain some functionality, will push it as your suggest
thanks

@vinothchandar
Copy link
Member

sg! @wangxianghu

I ll let @leesf or @yanghua do the merging once they are ready

@leesf
Copy link
Contributor

leesf commented Oct 14, 2020

sg! @wangxianghu

I ll let @leesf or @yanghua do the merging once they are ready

Merging this PR. cc @vinothchandar

@leesf leesf merged commit c7d962e into apache:master Oct 14, 2020
prashantwason pushed a commit to prashantwason/incubator-hudi that referenced this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants