-
Notifications
You must be signed in to change notification settings - Fork 468
[FLINK-36876] Add proxy for restClient #956
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
base: main
Are you sure you want to change the base?
Conversation
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.
Makes sense on a high level, reflection is not great but it's okay if we have a better solution in newer Flink versions.
Which Flink version will contain the Fix for this?
It depends on when will the pr apache/flink#25788 could be merged. If necessary, I can submit the pr fix in both 1.20 and 2.x versions of Flink. |
Is there any other problem with this pr? I don't know why CI check was cancelled |
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.
Added a minor clarifying question, also please rebase to retrigger the CI
this.groupField = groupField; | ||
|
||
// close previous group | ||
bootstrap.config().group().shutdown(); |
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.
How will this actually solve the original problem? The rest client still creates an individual group (that we close here) won't that cause the same issue anyways?
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.
Because PoolThreadCache is a request-thread-related cache, it is created and initialized when a request is made using the NioEventLoopGroup;
here it is created and closed immediately, with no requests in between, so there is no PoolThreadCache leakage issue.
Could you help take a look at the latest CI failure issue, which doesn't seem to be related to this code commit? |
What is the purpose of the change
Add proxy for restClient, which supports to use shared
EventLoopGroup
Brief change log
RestClientProxy
as the proxy of restClientkubernetes.operator.flink.client.io.threads
to specify the total thread of sharded eventLoopGroupVerifying this change
Unit tests + manual verification
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors
: (no)Documentation