-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Feature][Flink] Add external configuration parameters #5480
Conversation
LGTM, please add doc about this. |
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.
LGTM
There is a problem with some parameter configuration. I am working on it. Do not merge it |
Waiting CI/CD and offer some screenshots to verify it worked. |
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.
Please fix dead links in your docs.
PTAL: @TyrantLucifer @ruanwenjun I added some CI |
Overall LGTM, but the test case that you added I didn't any way to see the parameter worded really. Could you please compiled the project and try verify it in a local flink env and offer the screen shots for it? |
conf file
TestFlinkParameterorg.apache.seatunnel.example.flink.v2.SeaTunnelApiExample
|
I think you should ensure the parameters injected in real flink jobs, not only codes. You should submit a seatunnel on flink engine and make a snapshot to verify it worked. |
I provide some screenshots of the flink run web Ui |
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.
LGTM
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.
+1
PTAL: @Hisoka-X |
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.
Looking great! Thanks @zhilinli123 .
@TyrantLucifer Could you take a look again? |
LGTM |
Purpose of this pull request
Check list
New License Guide
release-note
.