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.

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

[cdc-common] Introduce PIPELINE_LOCAL_TIME_ZONE which help handle time zone well #2797

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

leonardBang
Copy link
Contributor

@leonardBang leonardBang commented Dec 2, 2023

[cdc-common] Introduce PIPELINE_LOCAL_TIME_ZONE which help handle time zone well

Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

Where is getLocalTimeZone invoked?

@leonardBang
Copy link
Contributor Author

Where is getLocalTimeZone invoked?

When we deal TIMESTAMP_LTZ type in Sink

@leonardBang
Copy link
Contributor Author

@lvyanquan @PatrickRen @Jiabao-Sun Would you like to help review this PR?

@leonardBang
Copy link
Contributor Author

Thanks @PatrickRen and @lvyanquan for the insight, I've simplify the PR, appreciate if you can take a look

Copy link
Contributor

@lvyanquan lvyanquan left a comment

Choose a reason for hiding this comment

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

Left one comment.

context.getConfiguration().get(ValuesDataSinkOptions.PRINT_ENABLED));
context.getPipelineConfiguration()
.get(ValuesDataSinkOptions.MATERIALIZED_IN_MEMORY),
context.getPipelineConfiguration().get(ValuesDataSinkOptions.PRINT_ENABLED));
Copy link
Contributor

Choose a reason for hiding this comment

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

context.getFactoryConfiguration().

Copy link
Contributor

Choose a reason for hiding this comment

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

And please fix the code style check failure. Thanks.

adress comments
Copy link
Contributor

@lvyanquan lvyanquan left a comment

Choose a reason for hiding this comment

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

LGTM.

@leonardBang leonardBang changed the title [cdc-common] Introduce PipelineConfig and PIPELINE_LOCAL_TIME_ZONE which help handle time zone well [cdc-common] Introduce PIPELINE_LOCAL_TIME_ZONE which help handle time zone well Dec 4, 2023
@leonardBang leonardBang merged commit 95921d5 into apache:master Dec 4, 2023
15 of 17 checks passed
@leonardBang
Copy link
Contributor Author

The failed tests is note related to this PR, merging...

@leonardBang leonardBang self-assigned this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants