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
[FLINK-30554][sql-client] Remove useless sessionId in the Executor #21597
Conversation
@flinkbot run azure |
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 for the contribution @fsk119 , I left some comments
...link-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
Outdated
Show resolved
Hide resolved
...link-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
Outdated
Show resolved
Hide resolved
...link-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
Outdated
Show resolved
Hide resolved
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java
Outdated
Show resolved
Hide resolved
...link-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
Outdated
Show resolved
Hide resolved
...link-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/LocalExecutor.java
Show resolved
Hide resolved
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java
Outdated
Show resolved
Hide resolved
*/ | ||
Map<String, String> getSessionConfigMap(String sessionId) throws SqlExecutionException; |
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.
I have one question. Original LocalExecutor
implements Executor
have a map ConcurrentHashMap<String, SessionContext> contextMap
, but now, we just delete it. Does this mean that each executor can only have one session? (If that, why we need sessionId, we can just define an executorId instead ? ). Also, I think if you delete this contextMap
, you need add tests to verify we don't need this map. WDYT ?
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.
Does this mean that each executor can only have one session?
Yes, you are right. Executor only works for one session in the new design. The Gateway is designed for the multitenancy.
why we need sessionId, we can just define an executorId instead?
I think session id is still useful. For example, SQL Client can convey the user name/password with the current API. So I am prone to keep it unchanged and prepare for the future authentication mechanism.
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.
Ok, I got it.
@flinkbot run azure |
05d32c2
to
23fba07
Compare
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 +1
What is the purpose of the change
Remove useless sessionId in the Executor. The SQL Gateway uses the
SessionHandle
to identify each Session and theSessionHandle
is assigned by the Gateway only. TheExecutor
in the SqlClient is the client to submit messages to theGateway and it is only used by the current Session. We don't need to use the sessionId everywhere.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation