Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[HUDI-5780] Refactor Deltastreamer source configs to use HoodieConfig/ConfigProperty #8184

Merged
merged 3 commits into from
Apr 2, 2023
Merged

[HUDI-5780] Refactor Deltastreamer source configs to use HoodieConfig/ConfigProperty #8184

merged 3 commits into from
Apr 2, 2023

Conversation

lokeshj1703
Copy link
Contributor

Change Logs

Relevant classes:

HiveIncrPullSource
HoodieIncrSource
JdbcSource
PulsarSource
S3EventsHoodieIncrSource
SqlSource
CloudObjectsSelector
CloudStoreIngestionConfig
DatePartitionPathSelector
DFSPathSelector
KafkaOffsetGen
GcsIngestionConfig
MaxwellJsonKafkaSourcePostProcessor
org.apache.hudi.utilities.testutils.sources.config.SourceConfigs

Impact

NA

Risk level (write none, low medium or high below)

low

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@lokeshj1703 lokeshj1703 changed the title Hudi 5780 [HUDI-5780] Refactor Deltastreamer source configs to use HoodieConfig/ConfigProperty Mar 14, 2023
@lokeshj1703 lokeshj1703 marked this pull request as ready for review March 14, 2023 16:35
@codope codope added priority:major degraded perf; unable to move forward; potential bugs hudistreamer issues related to Hudi streamer (Formely deltastreamer) configs code-refactor labels Mar 20, 2023
@lokeshj1703
Copy link
Contributor Author

Need to address #8152 (comment)

xushiyan
xushiyan reviewed Mar 26, 2023
View reviewed changes
hudi-utilities/src/main/java/org/apache/hudi/utilities/config/CloudSourceConfig.java
groupName = ConfigGroups.Names.DELTA_STREAMER,
subGroupName = ConfigGroups.SubGroupNames.DELTA_STREAMER_SOURCE,
description = "Configs that are common during ingestion across different cloud stores")
public class CloudSourceConfig extends HoodieConfig {
Copy link
Member

Choose a reason for hiding this comment

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

GCS event incr source is using this one and S3 event incr source is using S3EventsHoodieIncrSourceConfig. We should merge the latter into the former.

For S3 event incr source, we should check CloudSourceConfig first then fallback to the keys in S3EventsHoodieIncrSourceConfig, which should be marked as deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take this separately. This refactoring PR does not intend to change any behavior and purely for moving configs to use ConfigProperty class.

@lokeshj1703 could you file JIRA to track this?

Copy link
Member

Choose a reason for hiding this comment

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

Filed https://issues.apache.org/jira/browse/HUDI-6020

hudi-utilities/src/main/java/org/apache/hudi/utilities/config/CloudSourceConfig.java
.withDocumentation("Ignore objects in the bucket whose relative path contains this substring");

public static final ConfigProperty<String> SPARK_DATASOURCE_OPTIONS = ConfigProperty
.key("hoodie.deltastreamer.source.cloud.data.datasource.options")
Copy link
Member

Choose a reason for hiding this comment

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

a precise name would be hoodie.deltastreamer.source.cloud_incr.spark.datasource.options, at least we should highlight "spark". We should use newer name and keep the old one as deprecated alternative values.

Also refer to keys from S3EventsHoodieIncrSourceConfig

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. @lokeshj1703 let's track this in a new JIRA ticket.

Copy link
Member

Choose a reason for hiding this comment

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

Filed https://issues.apache.org/jira/browse/HUDI-6020

@yihua yihua mentioned this pull request Mar 30, 2023
Closed
4 tasks
yihua
yihua approved these changes Apr 1, 2023
View reviewed changes
Copy link
Contributor

@yihua yihua left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up @lokeshj1703 quite some work of refactoring. LGTM overall. I left a few minor comments which I'll address before merging the PR.

hudi-utilities/src/main/java/org/apache/hudi/utilities/config/CloudSourceConfig.java
groupName = ConfigGroups.Names.DELTA_STREAMER,
subGroupName = ConfigGroups.SubGroupNames.DELTA_STREAMER_SOURCE,
description = "Configs that are common during ingestion across different cloud stores")
public class CloudSourceConfig extends HoodieConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take this separately. This refactoring PR does not intend to change any behavior and purely for moving configs to use ConfigProperty class.

@lokeshj1703 could you file JIRA to track this?

hudi-utilities/src/main/java/org/apache/hudi/utilities/config/CloudSourceConfig.java
.withDocumentation("Ignore objects in the bucket whose relative path contains this substring");

public static final ConfigProperty<String> SPARK_DATASOURCE_OPTIONS = ConfigProperty
.key("hoodie.deltastreamer.source.cloud.data.datasource.options")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. @lokeshj1703 let's track this in a new JIRA ticket.

hudi-utilities/src/main/java/org/apache/hudi/utilities/config/GCSEventsSourceConfig.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/GcsEventsHoodieIncrSource.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/JdbcSource.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/helpers/KafkaOffsetGen.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/SqlSource.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/config/SqlSourceConfig.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/config/HoodieIncrSourceConfig.java Outdated Show resolved Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/config/SourceTestConfig.java Outdated Show resolved Hide resolved
@hudi-bot
Copy link

hudi-bot commented Apr 1, 2023

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@yihua yihua merged commit 3540119 into apache:master Apr 2, 2023
stayrascal pushed a commit to stayrascal/hudi that referenced this pull request Apr 20, 2023
@lokeshj1703 @yihua
[HUDI-5780] Refactor Deltastreamer source configs to use HoodieConfig…
5c7a8d8 
…/ConfigProperty (apache#8184)

This commit refactors Deltastreamer source configs to use HoodieConfig/ConfigProperty.  The following classes are relevant:

HiveIncrPullSource
HoodieIncrSource
JdbcSource
PulsarSource
S3EventsHoodieIncrSource
SqlSource
CloudObjectsSelector
CloudStoreIngestionConfig
DatePartitionPathSelector
DFSPathSelector
KafkaOffsetGen
GcsIngestionConfig
MaxwellJsonKafkaSourcePostProcessor

Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xushiyan xushiyan xushiyan left review comments

@yihua yihua yihua approved these changes

Assignees
No one assigned
Labels
code-refactor configs hudistreamer issues related to Hudi streamer (Formely deltastreamer) priority:major degraded perf; unable to move forward; potential bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lokeshj1703 @hudi-bot @yihua @xushiyan @codope