yuzelin commented on code in PR #21133:
URL: https://github.com/apache/flink/pull/21133#discussion_r1022361091
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/service/SqlGatewayServiceImpl.java:
##########
@@ -79,6 +82,32 @@ public void closeSession(SessionHandle sessionHandle) throws
SqlGatewayException
}
}
+ @Override
+ public ResultSet configureSession(
+ SessionHandle sessionHandle, String statement, long
executionTimeoutMs)
+ throws SqlGatewayException {
+ try {
+ if (executionTimeoutMs > 0) {
+ // TODO: support the feature in FLINK-27838
Review Comment:
> As a small detail,
[FLINK-27838](https://issues.apache.org/jira/browse/FLINK-27838) says it is
about a timeout for `executeStatement`. Should the ticket be updated to note
that `configureSession` also has a timeout?
Yes, I will add description of `configureSession` to that ticket.
> As another thought, in the future, if a `configureSession` call _does
timeout_, it seems like it could be challenging to manage rolling back the
configuration change as new requests are coming in.
Yes, you are right. I have some thoughts about this problem. Let's see why
we need `configureSession` since the `executeStatement` can submit the same
statement. This method is for the client to execute initialization file
(mentioned above), and the client will be closed if any error occurs before the
execution of initialization file finished. In this scenario we doesn't need to
consider the rolling back. If in other scenarios this method should be invoked
in the future, I think the caller should handle the error, such as retry or
restart mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
