stevelorddremio commented on code in PR #34817:
URL: https://github.com/apache/arrow/pull/34817#discussion_r1382089174
##########
format/Flight.proto:
##########
@@ -503,3 +504,100 @@ message FlightData {
message PutResult {
bytes app_metadata = 1;
}
+
+/*
+ * Request message for the "Close Session" action.
+ *
+ * The exiting session is referenced via a cookie header.
+ */
+message CloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message CloseSessionResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The session close status is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ CLOSE_RESULT_UNSPECIFIED = 0;
+ // The session close request is complete. Subsequent requests with
+ // a NOT_FOUND error.
+ CLOSE_RESULT_CLOSED = 1;
+ // The session close request is in progress. The client may retry
+ // the close request.
+ CLOSE_RESULT_CLOSING = 2;
+ // The session is not closeable. The client should not retry the
+ // close request.
+ CLOSE_RESULT_NOT_CLOSEABLE = 3;
+ }
+
+ Status status = 1;
+}
+
+message SessionOptionValue {
+ option (experimental) = true;
+
+ message StringListValue {
+ repeated string values = 1;
+ }
+
+ oneof option_value {
+ string string_value = 1;
+ bool bool_value = 2;
+ sfixed32 int32_value = 3;
+ sfixed64 int64_value = 4;
+ float float_value = 5;
+ double double_value = 6;
+ StringListValue string_list_value = 7;
+ }
+}
+
+message SetSessionOptionsRequest {
+ option (experimental) = true;
+
+ map<string, SessionOptionValue> session_options = 1;
+}
+
+message SetSessionOptionsResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The status of setting the option is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ SET_SESSION_OPTION_RESULT_UNSPECIFIED = 0;
+ // The session option setting completed successfully.
+ SET_SESSION_OPTION_RESULT_OK = 1;
Review Comment:
Do we need a specific ok status if a session option is overwritten with a
new value?
It might be intentional to overwrite but can be one of those gotchas if
unintentional and appeared to succeed.
##########
format/Flight.proto:
##########
@@ -503,3 +504,100 @@ message FlightData {
message PutResult {
bytes app_metadata = 1;
}
+
+/*
+ * Request message for the "Close Session" action.
+ *
+ * The exiting session is referenced via a cookie header.
+ */
+message CloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message CloseSessionResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The session close status is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ CLOSE_RESULT_UNSPECIFIED = 0;
+ // The session close request is complete. Subsequent requests with
Review Comment:
2nd sentence doesn't quite make sense. Does the following convey the correct
action:
Send a NOT_FOUND error on subsequent requests.
##########
cpp/src/arrow/flight/client.h:
##########
@@ -383,6 +383,27 @@ class ARROW_FLIGHT_EXPORT FlightClient {
return DoExchange({}, descriptor);
}
+ /// \\brief Set server session option(s) by key/value. Sessions are generally
+ /// persisted via HTTP cookies.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The server session options to set
+ ::arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const FlightCallOptions& options, const SetSessionOptionsRequest&
request);
+
+ /// \\brief Get the current server session options. The session is generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The GetSessionOptions request object.
+ ::arrow::Result<GetSessionOptionsResult> GetSessionOptions(
+ const FlightCallOptions& options, const GetSessionOptionsRequest&
request);
Review Comment:
GetSessionOptionsRequest is empty in the proto definition. Is it there for
future expansion?
##########
format/Flight.proto:
##########
@@ -503,3 +504,100 @@ message FlightData {
message PutResult {
bytes app_metadata = 1;
}
+
+/*
+ * Request message for the "Close Session" action.
+ *
+ * The exiting session is referenced via a cookie header.
+ */
+message CloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message CloseSessionResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The session close status is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ CLOSE_RESULT_UNSPECIFIED = 0;
+ // The session close request is complete. Subsequent requests with
+ // a NOT_FOUND error.
+ CLOSE_RESULT_CLOSED = 1;
+ // The session close request is in progress. The client may retry
+ // the close request.
+ CLOSE_RESULT_CLOSING = 2;
+ // The session is not closeable. The client should not retry the
+ // close request.
+ CLOSE_RESULT_NOT_CLOSEABLE = 3;
+ }
+
+ Status status = 1;
+}
+
+message SessionOptionValue {
+ option (experimental) = true;
+
+ message StringListValue {
+ repeated string values = 1;
+ }
+
+ oneof option_value {
+ string string_value = 1;
+ bool bool_value = 2;
+ sfixed32 int32_value = 3;
+ sfixed64 int64_value = 4;
+ float float_value = 5;
+ double double_value = 6;
+ StringListValue string_list_value = 7;
+ }
+}
+
+message SetSessionOptionsRequest {
+ option (experimental) = true;
+
+ map<string, SessionOptionValue> session_options = 1;
+}
+
+message SetSessionOptionsResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The status of setting the option is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ SET_SESSION_OPTION_RESULT_UNSPECIFIED = 0;
+ // The session option setting completed successfully.
+ SET_SESSION_OPTION_RESULT_OK = 1;
+ // The given session option name was an alias for another option name.
+ SET_SESSION_OPTION_RESULT_OK_MAPPED = 2;
+ // The given session option name is invalid.
+ SET_SESSION_OPTION_RESULT_INVALID_NAME = 3;
+ // The session cannot be set to the given value.
+ SET_SESSION_OPTION_RESULT_INVALID_VALUE = 4;
+ // The session cannot be set.
+ SET_SESSION_OPTION_RESULT_ERROR = 5;
Review Comment:
Minor wording correction: "The session *option* cannot be set."
##########
cpp/src/arrow/flight/client.h:
##########
@@ -383,6 +383,27 @@ class ARROW_FLIGHT_EXPORT FlightClient {
return DoExchange({}, descriptor);
}
+ /// \\brief Set server session option(s) by key/value. Sessions are generally
+ /// persisted via HTTP cookies.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The server session options to set
+ ::arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const FlightCallOptions& options, const SetSessionOptionsRequest&
request);
+
+ /// \\brief Get the current server session options. The session is generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The GetSessionOptions request object.
Review Comment:
Since this is an empty object, shouldn't the comment reflect its purpose,
which is to do nothing?
##########
cpp/src/arrow/flight/client.h:
##########
@@ -383,6 +383,27 @@ class ARROW_FLIGHT_EXPORT FlightClient {
return DoExchange({}, descriptor);
}
+ /// \\brief Set server session option(s) by key/value. Sessions are generally
+ /// persisted via HTTP cookies.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The server session options to set
+ ::arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const FlightCallOptions& options, const SetSessionOptionsRequest&
request);
+
+ /// \\brief Get the current server session options. The session is generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The GetSessionOptions request object.
+ ::arrow::Result<GetSessionOptionsResult> GetSessionOptions(
+ const FlightCallOptions& options, const GetSessionOptionsRequest&
request);
+
+ /// \\brief Close/invalidate the current server session. The session is
generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The CloseSession request object.
+ ::arrow::Result<CloseSessionResult> CloseSession(const FlightCallOptions&
options,
+ const CloseSessionRequest&
request);
Review Comment:
CloseSessionRequest is empty in the proto definition. Is it there for future
expansion?
##########
format/Flight.proto:
##########
@@ -503,3 +504,100 @@ message FlightData {
message PutResult {
bytes app_metadata = 1;
}
+
+/*
+ * Request message for the "Close Session" action.
+ *
+ * The exiting session is referenced via a cookie header.
+ */
+message CloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message CloseSessionResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The session close status is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ CLOSE_RESULT_UNSPECIFIED = 0;
+ // The session close request is complete. Subsequent requests with
+ // a NOT_FOUND error.
+ CLOSE_RESULT_CLOSED = 1;
+ // The session close request is in progress. The client may retry
+ // the close request.
+ CLOSE_RESULT_CLOSING = 2;
+ // The session is not closeable. The client should not retry the
+ // close request.
+ CLOSE_RESULT_NOT_CLOSEABLE = 3;
+ }
+
+ Status status = 1;
+}
+
+message SessionOptionValue {
+ option (experimental) = true;
+
+ message StringListValue {
+ repeated string values = 1;
+ }
+
+ oneof option_value {
+ string string_value = 1;
+ bool bool_value = 2;
+ sfixed32 int32_value = 3;
+ sfixed64 int64_value = 4;
+ float float_value = 5;
+ double double_value = 6;
+ StringListValue string_list_value = 7;
+ }
+}
+
+message SetSessionOptionsRequest {
+ option (experimental) = true;
+
+ map<string, SessionOptionValue> session_options = 1;
+}
+
+message SetSessionOptionsResult {
+ option (experimental) = true;
+
+ enum Status {
+ // The status of setting the option is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ SET_SESSION_OPTION_RESULT_UNSPECIFIED = 0;
+ // The session option setting completed successfully.
+ SET_SESSION_OPTION_RESULT_OK = 1;
+ // The given session option name was an alias for another option name.
+ SET_SESSION_OPTION_RESULT_OK_MAPPED = 2;
+ // The given session option name is invalid.
+ SET_SESSION_OPTION_RESULT_INVALID_NAME = 3;
+ // The session cannot be set to the given value.
Review Comment:
Minor wording correction: "The session *option* cannot be set to the given
value."
##########
cpp/src/arrow/flight/client.h:
##########
@@ -383,6 +383,27 @@ class ARROW_FLIGHT_EXPORT FlightClient {
return DoExchange({}, descriptor);
}
+ /// \\brief Set server session option(s) by key/value. Sessions are generally
+ /// persisted via HTTP cookies.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The server session options to set
+ ::arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const FlightCallOptions& options, const SetSessionOptionsRequest&
request);
+
+ /// \\brief Get the current server session options. The session is generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The GetSessionOptions request object.
+ ::arrow::Result<GetSessionOptionsResult> GetSessionOptions(
+ const FlightCallOptions& options, const GetSessionOptionsRequest&
request);
+
+ /// \\brief Close/invalidate the current server session. The session is
generally
+ /// accessed via an HTTP cookie.
+ /// \param[in] options Per-RPC options
+ /// \param[in] request The CloseSession request object.
Review Comment:
Should this be clearer that this has no functional impact?
--
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]
