-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-17789][API/Core] DelegatingConfiguration should remove prefix … #12905
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
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit c9e05d7 (Wed Jul 15 11:11:37 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
|
Hi @JingsongLi , Looking forward to your review, thanks. |
|
Hi @zentol , Thanks for your commits. Can you review this PR for us ? |
Thanks @JingsongLi for your reply. |
zentol
left a comment
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.
Probably fine.
I thought that changing this would break something for the formats, but tests are passing and I cannot find where I draw this conclusion from.
|
@flinkbot run azure |
|
@pyscala Can you rebase for triggering testing? |
@JingsongLi I will do it later |
|
Hi @JingsongLi |
|
@pyscala You haven't rebase on the latest master, and are about a week behind. |
|
Hi @JingsongLi @zentol Thanks for detailed reply. is this OK ? Or I close this PR and create a new one ? |
|
@pyscala You can squash your commits, and cherry-pick to latest master, then replace your |
|
Hi @JingsongLi @zentol Done, Everything looks good |
JingsongLi
left a comment
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.
Thanks @pyscala for your updating. Looks good to me.
Merging...
What is the purpose of the change
1.DelegatingConfiguration should remove prefix instead of add prefix in toMap
2.Make
toMapconsistent withaddAllToPropertiesin classDelegatingConfigurationBrief change log
1.DelegatingConfiguration should remove prefix instead of add prefix in toMap
2.Make
toMapconsistent withaddAllToPropertiesin classDelegatingConfigurationVerifying this change
This change added tests and can be verified as follows:
unit test
testDelegationConfigurationToMapConsistentWithAddAllToPropertiesin classDelegatingConfigurationTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation