-
Notifications
You must be signed in to change notification settings - Fork 478
[Flink] Add RuntimeContextAdapter #2241
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
vamossagar12
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.
LGTM
wuchong
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 @sd4324530 , could you add Unit Tests or Integration Tests to reproduce this issue? So that we won't break this compatibility in the future.
Others looks good to me.
|
@wuchong Fixd. Please take a look. |
|
@sd4324530 thank you. Could you also add this test for flink 1.18 and flink 1.19? This helps verify the added adapter in Flink 1.18 is correct, and not adding adapter in Flink 1.19 is also correct. Besides, could you use rebase to rebase your branch onto |
…nk-common` Signed-off-by: peiyu <125331682@qq.com>
Signed-off-by: Pei Yu <125331682@qq.com>
622a979 to
3654d56
Compare
|
|
||
| @BeforeEach | ||
| @Override | ||
| void beforeEach() throws Exception { |
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.
Do not override parent test method, otherwise, it's hard to maintain. If the Flink classes used in test are also not compatible across different Flink versions, we should do the same adapter for them (but leave them in test packages only), you can checkout ResolvedCatalogMaterializedTableAdapter for an example.
Signed-off-by: Pei Yu <125331682@qq.com>
Purpose
Linked issue: close #2238
Brief change log
add RuntimeContextAdapter in module
fluss-flink-1.18andfluss-flink-common