[HUDI-1122] Introduce a kafka implementation of hoodie write commit ca… by wangxianghu · Pull Request #1886 · apache/hudi

wangxianghu · 2020-07-29T13:22:07Z

…llback

Tips

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

What is the purpose of the pull request

Introduce a kafka implementation of hoodie write commit callback

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.

wangxianghu · 2020-07-29T13:23:58Z

@yanghua please take a look when free

yanghua

@Mathieu1124 Thanks for your contribution. Left some comments you can consider. Did you test this feature in your local?

yanghua · 2020-08-03T00:47:08Z

hudi-client/src/main/java/org/apache/hudi/callback/util/HoodieWriteCommitCallbackUtil.java

If you do not want to add the comments for @param and @return. Just remove them.

If you do not want to add the comments for @param and @return. Just remove them.

yes, will remove it

yanghua · 2020-08-03T00:51:19Z

hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteCommitCallbackConfig.java

I am thinking one thing: the config options of the callback feature for Kafka are left in hudi-client, while the implementation hosts in hudi-utilities, if it's suitable.

I am thinking one thing: the config options of the callback feature for Kafka are left in hudi-client, while the implementation hosts in hudi-utilities, if it's suitable.

It is truly a problem. since kafka callback implementation is placed in hudi-utilities, the config should be there too. will move it

yanghua · 2020-08-03T00:53:45Z

...s/src/main/java/org/apache/hudi/utilities/callback/kafka/HoodieWriteCommitKafkaCallback.java

Shall we use try-with-resource or try-catch block to prevent leaking resources?

Shall we use try-with-resource or try-catch block to prevent leaking resources?

yes, will do

yanghua · 2020-08-03T00:55:20Z

...c/main/java/org/apache/hudi/utilities/callback/kafka/HoodieWriteCommitKafkaCallbackUtil.java

IMO, moving the content of this class into HoodieWriteCommitKafkaCallback is reasonable. WDYT?

IMO, moving the content of this class into HoodieWriteCommitKafkaCallback is reasonable. WDYT?

I once hesitated here, kind of messy to host in one class. But it is reasonable, will move

wangxianghu · 2020-08-03T02:40:33Z

@Mathieu1124 Thanks for your contribution. Left some comments you can consider. Did you test this feature in your local?

Yes, I tested it in my local, it works good.

yanghua

@Mathieu1124 Thanks for addressing my concerns. Left one comment.

yanghua · 2020-08-05T13:00:37Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java

If users configure http callback? Why we only consider Kafka callback here?

If users configure http callback? Why we only consider Kafka callback here?

the config of http callback will be Initialized when HoodieWriteClient set up, while kafka's can't. the kafka's config can only be set up in hudi-utilities module, so the logic above come up.

wangxianghu · 2020-08-05T15:30:41Z

Hi @vinothchandar, I was wondering can we move this implement to hudi-client module just like the way all the implementations of metrics does.

put this implementation in hudi-utilities, the Spark API(hudi-spark moudle) can not benefit from it. besides, in the future, when the flink engine merged in , it can not use it either.
WDYT?

wangxianghu · 2020-08-07T03:00:05Z

@yanghua VC seems busy, do you have any other concern about this pr ?
if it is ok, can we merge this first, and file a new pr if VC agree to move this to hudi-client in the future :)

vinothchandar · 2020-08-07T06:46:23Z

As the RM sent out the note, we are only landing small PRs and release blockers, so we can keep master stable for cutting the 0.6.0 RC. Apologize for the inconvenience.

vinothchandar · 2020-08-07T06:49:04Z

I was wondering can we move this implement to hudi-client module just like the way all the implementations of metrics does.

I think we can move this down the line. hudi-client or hudi-spark talking a direct dependency on kafka does not feel that clean to me. May be file a follow up JIRA?

wangxianghu · 2020-08-07T10:40:49Z

I was wondering can we move this implement to hudi-client module just like the way all the implementations of metrics does.

I think we can move this down the line. hudi-client or hudi-spark talking a direct dependency on kafka does not feel that clean to me. May be file a follow up JIRA?

ok

…llback

yanghua

@wangxianghu Left some new comments.

yanghua · 2020-08-17T07:57:58Z

hudi-client/src/main/java/org/apache/hudi/callback/util/HoodieWriteCommitCallbackUtil.java

+   */
+  public static String convertToJsonString(Object obj) {
+    // convert to json
+    ObjectMapper mapper = new ObjectMapper();


Can we make this variable static to be shared in the whole class?

yanghua · 2020-08-17T07:59:17Z

...s/src/main/java/org/apache/hudi/utilities/callback/kafka/HoodieWriteCommitKafkaCallback.java

+      producer.send(record);
+      LOG.info(String.format("Send callback message %s succeed", callbackMsg));
+    } catch (Exception e) {
+      LOG.error("Send kafka callback msg failed : " + e);


LOG.error("Send kafka callback msg failed : "，e);

yanghua · 2020-08-17T07:59:50Z

...s/src/main/java/org/apache/hudi/utilities/callback/kafka/HoodieWriteCommitKafkaCallback.java

+    kafkaProducerProps.setProperty(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG,
+        "org.apache.kafka.common.serialization.StringSerializer");
+
+    LOG.info("Callback kafka producer init with configs: "


Using LOG.debug?

yanghua · 2020-08-17T08:01:01Z

...s/src/main/java/org/apache/hudi/utilities/callback/kafka/HoodieWriteCommitKafkaCallback.java

+          props.getProperty(TABLE_NAME),
+          callbackMsg);
+    } else {
+      return new ProducerRecord<String, String>(topic,


Maybe we do not need to break the lines here?

yanghua

@wangxianghu Thanks for addressing my concerns. LGTM, will merge it if CI is OK.

yanghua · 2020-08-20T15:00:43Z

I was wondering can we move this implement to hudi-client module just like the way all the implementations of metrics does.

I think we can move this down the line. hudi-client or hudi-spark talking a direct dependency on kafka does not feel that clean to me. May be file a follow up JIRA?

@wangxianghu Please follow vc's suggestion, file a Jira issue to track the future work.

wangxianghu · 2020-08-21T01:38:07Z

hudi-client or hudi-spark talking a direct dependency on kafka

@yanghua thanks for your time. The ticket filed here: https://issues.apache.org/jira/browse/HUDI-1207

wangxianghu force-pushed the HUDI-1122 branch 2 times, most recently from 916cf08 to 6cc6bd5 Compare July 29, 2020 13:58

yanghua self-assigned this Jul 30, 2020

yanghua self-requested a review July 30, 2020 01:31

yanghua reviewed Aug 3, 2020

View reviewed changes

wangxianghu force-pushed the HUDI-1122 branch 2 times, most recently from c6cf2ed to 4fd9416 Compare August 4, 2020 15:12

yanghua reviewed Aug 5, 2020

View reviewed changes

wangxianghu force-pushed the HUDI-1122 branch 2 times, most recently from 3a549b3 to 79f73ba Compare August 9, 2020 09:37

[HUDI-1122]Introduce a kafka implementation of hoodie write commit ca…

1a76592

…llback

wangxianghu force-pushed the HUDI-1122 branch from 79f73ba to 1a76592 Compare August 9, 2020 10:05

wangxianghu mentioned this pull request Aug 17, 2020

[HUDI-1186][DOC] Add description of write commit callback by kafka to document #1974

Merged

5 tasks

yanghua requested changes Aug 17, 2020

View reviewed changes

resolve style issue

7c3210e

yanghua approved these changes Aug 17, 2020

View reviewed changes

yanghua changed the title ~~[HUDI-1122]Introduce a kafka implementation of hoodie write commit ca…~~ [HUDI-1122] Introduce a kafka implementation of hoodie write commit ca… Aug 17, 2020

yanghua merged commit b883b6d into apache:master Aug 20, 2020

wangxianghu deleted the HUDI-1122 branch August 28, 2020 10:14

Conversation

wangxianghu commented Jul 29, 2020

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

wangxianghu commented Jul 29, 2020

Uh oh!

yanghua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxianghu commented Aug 3, 2020

Uh oh!

yanghua left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxianghu commented Aug 5, 2020

Uh oh!

wangxianghu commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinothchandar commented Aug 7, 2020

Uh oh!

vinothchandar commented Aug 7, 2020

Uh oh!

wangxianghu commented Aug 7, 2020

Uh oh!

yanghua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanghua left a comment

Choose a reason for hiding this comment

Uh oh!

yanghua commented Aug 20, 2020

Uh oh!

wangxianghu commented Aug 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

yanghua left a comment •

edited

Loading

wangxianghu commented Aug 7, 2020 •

edited

Loading