New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-1023] Add validation error messages in delta sync #1710
[HUDI-1023] Add validation error messages in delta sync #1710
Conversation
- avoid overwrite index type - add more validation error message - make write config constants public
60ce80b
to
22533af
Compare
hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1710 +/- ##
=========================================
Coverage 18.19% 18.20%
- Complexity 857 858 +1
=========================================
Files 348 348
Lines 15358 15357 -1
Branches 1525 1525
=========================================
+ Hits 2794 2795 +1
+ Misses 12206 12204 -2
Partials 358 358
Continue to review full report at Codecov.
|
.withCompactionConfig(HoodieCompactionConfig.newBuilder().withPayloadClass(cfg.payloadClassName) | ||
// Inline compaction is disabled for continuous mode. otherwise enabled for MOR | ||
.withInlineCompaction(cfg.isInlineCompactionEnabled()).build()) | ||
.forTable(cfg.targetTableName) | ||
.withIndexConfig(HoodieIndexConfig.newBuilder().withIndexType(HoodieIndex.IndexType.BLOOM).build()) | ||
.withAutoCommit(false).withProps(props); | ||
.withAutoCommit(autoCommit).withProps(props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this will override this.. the withProps()
. no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinothchandar you're right. it was my overlook. But i guess this line is redundant anyway as index is default to BLOOM, setting the index config may cause some confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to be nit-picky.. This PR just exceeded the 50 line threshold for minor.. Mind adding a ticket? :)
@vinothchandar sure. ticket added. |
@vinothchandar Is this good to merge? :) |
yes sir.. on it.. |
This change added tests and can be verified as follows:
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.