fsk119 commented on code in PR #21525:
URL: https://github.com/apache/flink/pull/21525#discussion_r1053229800
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/util/SqlGatewayRestAPIVersion.java:
##########
@@ -35,7 +35,8 @@
// The bigger the ordinal(its position in enum declaration), the higher
the level of the
// version.
V0(false, false),
- V1(true, true);
+ V1(true, true),
+ V2(false, true);
Review Comment:
Please do as Hive does and add some description about the change.
```
enum TProtocolVersion {
HIVE_CLI_SERVICE_PROTOCOL_V1,
// V2 adds support for asynchronous execution
HIVE_CLI_SERVICE_PROTOCOL_V2
// V3 add varchar type, primitive type qualifiers
HIVE_CLI_SERVICE_PROTOCOL_V3
// V4 add decimal precision/scale, char type
HIVE_CLI_SERVICE_PROTOCOL_V4
// V5 adds error details when GetOperationStatus returns in error state
HIVE_CLI_SERVICE_PROTOCOL_V5
// V6 uses binary type for binary payload (was string) and uses columnar
result set
HIVE_CLI_SERVICE_PROTOCOL_V6
// V7 adds support for delegation token based connection
HIVE_CLI_SERVICE_PROTOCOL_V7
// V8 adds support for interval types
HIVE_CLI_SERVICE_PROTOCOL_V8
// V9 adds support for serializing ResultSets in SerDe
HIVE_CLI_SERVICE_PROTOCOL_V9
// V10 adds support for in place updates via GetOperationStatus
HIVE_CLI_SERVICE_PROTOCOL_V10
// V11 adds timestamp with local time zone type
HIVE_CLI_SERVICE_PROTOCOL_V11
}
```
// V2 supports to configureSession and allow to serialize the RowData with
PLAIN_TEXT or JSON.
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/header/session/ConfigureSessionHeaders.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.gateway.rest.header.session;
+
+import org.apache.flink.runtime.rest.HttpMethodWrapper;
+import org.apache.flink.runtime.rest.messages.EmptyResponseBody;
+import org.apache.flink.runtime.rest.versioning.RestAPIVersion;
+import org.apache.flink.table.gateway.rest.header.SqlGatewayMessageHeaders;
+import
org.apache.flink.table.gateway.rest.message.session.ConfigureSessionRequestBody;
+import
org.apache.flink.table.gateway.rest.message.session.SessionHandleIdPathParameter;
+import
org.apache.flink.table.gateway.rest.message.session.SessionMessageParameters;
+import org.apache.flink.table.gateway.rest.util.SqlGatewayRestAPIVersion;
+
+import
org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus;
+
+import java.util.Collection;
+import java.util.Collections;
+
+/** Message headers for configuring a session. */
+public class ConfigureSessionHeaders
+ implements SqlGatewayMessageHeaders<
+ ConfigureSessionRequestBody, EmptyResponseBody,
SessionMessageParameters> {
+
+ private static final ConfigureSessionHeaders INSTANCE = new
ConfigureSessionHeaders();
+
+ private static final String URL =
+ "/sessions/:" + SessionHandleIdPathParameter.KEY +
"/configure_session";
Review Comment:
I think "/configure_session" is not very suitable. Because Flink currently
uses '-' to concat the words in the URL, e.g.
```
/taskmanagers/:taskmanagerid/thread-dump
```
Many documents also recommend us to use - rather than _ in the url.
[1] https://stackoverflow.com/questions/5362954/why-use-instead-off-in-url
[2]
https://studiohawk.com.au/blog/dash-or-underscore-in-url-heres-how-its-affecting-your-seo/
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/handler/session/ConfigureSessionHandler.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.gateway.rest.handler.session;
+
+import org.apache.flink.runtime.rest.handler.HandlerRequest;
+import org.apache.flink.runtime.rest.handler.RestHandlerException;
+import org.apache.flink.runtime.rest.messages.EmptyResponseBody;
+import org.apache.flink.runtime.rest.messages.MessageHeaders;
+import org.apache.flink.table.gateway.api.SqlGatewayService;
+import org.apache.flink.table.gateway.api.session.SessionHandle;
+import
org.apache.flink.table.gateway.rest.handler.AbstractSqlGatewayRestHandler;
+import
org.apache.flink.table.gateway.rest.message.session.ConfigureSessionRequestBody;
+import
org.apache.flink.table.gateway.rest.message.session.SessionHandleIdPathParameter;
+import
org.apache.flink.table.gateway.rest.message.session.SessionMessageParameters;
+import org.apache.flink.table.gateway.rest.util.SqlGatewayRestAPIVersion;
+
+import javax.annotation.Nonnull;
+
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+
+/** Handler to execute a statement. */
Review Comment:
Handler to configure a session with statement?
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/util/SqlGatewayRestAPIVersion.java:
##########
@@ -35,7 +35,8 @@
// The bigger the ordinal(its position in enum declaration), the higher
the level of the
// version.
V0(false, false),
- V1(true, true);
+ V1(true, true),
Review Comment:
I think V2 is the default version now.
##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/SessionRelatedITCase.java:
##########
@@ -85,11 +115,11 @@ void testCreateAndCloseSessions() throws Exception {
CompletableFuture<OpenSessionResponseBody> response =
sendRequest(openSessionHeaders, emptyParameters,
openSessionRequestBody);
String sessionHandleId = response.get().getSessionHandle();
- assertThat(sessionHandleId).isNotNull();
+ assertNotNull(sessionHandleId);
Review Comment:
Why modify this? I think `org.assertj.core.api.Assertions.assertThat` is
suggested test API.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
