lidavidm commented on code in PR #34817: URL: https://github.com/apache/arrow/pull/34817#discussion_r1453483602
########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/CloseSessionRequest.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.arrow.flight; + +import java.io.IOException; +import java.nio.ByteBuffer; + +import org.apache.arrow.flight.impl.Flight; + +/** A request to close/invalidate a server session context. */ +public class CloseSessionRequest { + public CloseSessionRequest() {} + + CloseSessionRequest(Flight.CloseSessionRequest proto) {} + + Flight.CloseSessionRequest toProtocol() { + Flight.CloseSessionRequest.Builder b = Flight.CloseSessionRequest.newBuilder(); Review Comment: nit: I believe there's a defaultInstance() or similar constructor ########## java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/SetSessionOptionsResultListener.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.arrow.flight.sql; + +import org.apache.arrow.flight.FlightProducer; +import org.apache.arrow.flight.Result; +import org.apache.arrow.flight.SetSessionOptionsResult; + +/** Typed StreamListener for renewFlightEndpoint. */ Review Comment: Docstring references the wrong endpoint ########## java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/GetSessionOptionsResultListener.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.arrow.flight.sql; + +import org.apache.arrow.flight.FlightProducer; +import org.apache.arrow.flight.GetSessionOptionsResult; +import org.apache.arrow.flight.Result; + +/** Typed StreamListener for renewFlightEndpoint. */ Review Comment: Docstring references the wrong endpoint. ########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/SessionOptionValue.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.arrow.flight; + +import java.util.Arrays; + +import org.apache.arrow.flight.impl.Flight; + +/** + * A union-like container interface for supported session option value types. + */ +public abstract class SessionOptionValue { Review Comment: Hmm, too bad we don't have sealed classes. We could have a package-private (empty) constructor so that you can't subclass this from outside the package? ########## format/Flight.proto: ########## @@ -525,3 +525,108 @@ message FlightData { message PutResult { bytes app_metadata = 1; } + +/* + * EXPERIMENTAL: Union of possible value types for a Session Option to be set to. + */ +message SessionOptionValue { + 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; + } +} + +/* + * EXPERIMENTAL: A request to set session options for an existing or new (implicit) + * server session. + * + * Sessions are persisted and referenced via RFC 6265 HTTP cookies, canonically + * 'arrow_flight_session_id', although implementations may freely choose their own name. + */ +message SetSessionOptionsRequest { + map<string, SessionOptionValue> session_options = 1; +} + +/* + * EXPERIMENTAL: The results (individually) of setting a set of session options. + */ +message SetSessionOptionsResult { + 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. + UNSPECIFIED = 0; + // The session option setting completed successfully. + OK = 1; + // The given session option name was an alias for another option name. + OK_MAPPED = 2; + // The given session option name is invalid. + INVALID_NAME = 3; + // The session option value is invalid. + INVALID_VALUE = 4; + // The session option cannot be set. + ERROR = 5; + } + + message Result { + Status status = 1; + } + + map<string, Result> results = 1; +} + +/* + * EXPERIMENTAL: A request to access the session options for the current server session. + * + * The existing session is referenced via a cookie header; it is an error to make this + * request with a missing, invalid, or expired session cookie header. + */ +message GetSessionOptionsRequest { +} + +/* + * EXPERIMENTAL: The result containing the current server session options. + */ +message GetSessionOptionsResult { + map<string, SessionOptionValue> session_options = 1; +} + +/* + * Request message for the "Close Session" action. + * + * The exiting session is referenced via a cookie header. + */ +message CloseSessionRequest { +} Review Comment: Was this spelled out anywhere? ########## java/flight/flight-integration-tests/pom.xml: ########## @@ -45,6 +45,10 @@ <groupId>com.google.protobuf</groupId> <artifactId>protobuf-java</artifactId> </dependency> + <dependency> Review Comment: nit: this is mis-indented ########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java: ########## @@ -0,0 +1,228 @@ +/* + * 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.arrow.flight; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Middleware for handling Flight SQL Sessions including session cookie handling. + * + * Currently experimental. + */ +public class ServerSessionMiddleware implements FlightServerMiddleware { + Factory factory; + boolean existingSession; + private Session session; + private String closedSessionId = null; + + public static final String sessionCookieName = "arrow_flight_session_id"; + + /** + * Factory for managing and accessing ServerSessionMiddleware. + */ + public static class Factory implements FlightServerMiddleware.Factory<ServerSessionMiddleware> { + private final Map<String, Session> sessionStore = + new ConcurrentHashMap<>(); + private final Callable<String> idGenerator; + + /** + * Construct a factory for ServerSessionMiddleware. + * + * Factory manages and accesses persistent sessions based on HTTP cookies. + * + * @param idGenerator A Callable returning unique session id Strings. + */ + public Factory(Callable<String> idGenerator) { + this.idGenerator = idGenerator; + } + + private synchronized Session createNewSession() { + String id; + try { + id = idGenerator.call(); + } catch (Exception ignored) { + // Most impls aren't going to throw so don't make caller handle a nonexistent checked exception + return null; + } + if (sessionStore.containsKey(id)) { + // Collision, should also never happen + return null; + } + + Session newSession = new Session(id); + sessionStore.put(id, newSession); Review Comment: This is a TOC/TOU error even if we expect it to never happen; instead use putIfAbsent and throw if the result is not null ########## format/Flight.proto: ########## @@ -525,3 +525,110 @@ message FlightData { message PutResult { bytes app_metadata = 1; } + +/* + * EXPERIMENTAL: Union of possible value types for a Session Option to be set to. + * + * By convention, an attempt to set a valueless SessionOptionValue should + * attempt to unset or clear the named option value on the server. + */ +message SessionOptionValue { + message StringListValue { + repeated string values = 1; + } + + oneof option_value { + string string_value = 1; + bool bool_value = 2; + sfixed64 int64_value = 3; + double double_value = 4; + StringListValue string_list_value = 5; + } +} + +/* + * EXPERIMENTAL: A request to set session options for an existing or new (implicit) + * server session. + * + * Sessions are persisted and referenced via a transport-level state management, typically + * RFC 6265 HTTP cookies when using an HTTP transport. The suggested cookie name or state + * context key is 'arrow_flight_session_id', although implementations may freely choose their + * own name. + */ +message SetSessionOptionsRequest { + map<string, SessionOptionValue> session_options = 1; +} + +/* + * EXPERIMENTAL: The results (individually) of setting a set of session options. + * + * Option names should only be present in the Review Comment: Cut off sentence ########## java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/CloseSessionResultListener.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.arrow.flight.sql; + +import org.apache.arrow.flight.CloseSessionResult; +import org.apache.arrow.flight.FlightProducer; +import org.apache.arrow.flight.Result; + +/** Typed StreamListener for renewFlightEndpoint. */ Review Comment: Docstring references wrong endpoint. ########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java: ########## @@ -0,0 +1,228 @@ +/* + * 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.arrow.flight; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Middleware for handling Flight SQL Sessions including session cookie handling. + * + * Currently experimental. + */ +public class ServerSessionMiddleware implements FlightServerMiddleware { + Factory factory; + boolean existingSession; + private Session session; + private String closedSessionId = null; + + public static final String sessionCookieName = "arrow_flight_session_id"; + + /** + * Factory for managing and accessing ServerSessionMiddleware. + */ + public static class Factory implements FlightServerMiddleware.Factory<ServerSessionMiddleware> { + private final Map<String, Session> sessionStore = + new ConcurrentHashMap<>(); + private final Callable<String> idGenerator; + + /** + * Construct a factory for ServerSessionMiddleware. + * + * Factory manages and accesses persistent sessions based on HTTP cookies. + * + * @param idGenerator A Callable returning unique session id Strings. + */ + public Factory(Callable<String> idGenerator) { + this.idGenerator = idGenerator; + } + + private synchronized Session createNewSession() { + String id; + try { + id = idGenerator.call(); + } catch (Exception ignored) { + // Most impls aren't going to throw so don't make caller handle a nonexistent checked exception + return null; Review Comment: Throw a runtime exception instead of swallowing it? Or else, declare our own functional interface that doesn't throw a checked exception ########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java: ########## @@ -0,0 +1,228 @@ +/* + * 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.arrow.flight; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Middleware for handling Flight SQL Sessions including session cookie handling. + * + * Currently experimental. + */ +public class ServerSessionMiddleware implements FlightServerMiddleware { + Factory factory; + boolean existingSession; + private Session session; + private String closedSessionId = null; + + public static final String sessionCookieName = "arrow_flight_session_id"; + + /** + * Factory for managing and accessing ServerSessionMiddleware. + */ + public static class Factory implements FlightServerMiddleware.Factory<ServerSessionMiddleware> { + private final Map<String, Session> sessionStore = Review Comment: nit: use ConcurrentMap as the abstract type? ########## java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java: ########## @@ -0,0 +1,228 @@ +/* + * 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.arrow.flight; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Middleware for handling Flight SQL Sessions including session cookie handling. + * + * Currently experimental. + */ +public class ServerSessionMiddleware implements FlightServerMiddleware { + Factory factory; + boolean existingSession; + private Session session; + private String closedSessionId = null; + + public static final String sessionCookieName = "arrow_flight_session_id"; + + /** + * Factory for managing and accessing ServerSessionMiddleware. + */ + public static class Factory implements FlightServerMiddleware.Factory<ServerSessionMiddleware> { + private final Map<String, Session> sessionStore = + new ConcurrentHashMap<>(); + private final Callable<String> idGenerator; + + /** + * Construct a factory for ServerSessionMiddleware. + * + * Factory manages and accesses persistent sessions based on HTTP cookies. + * + * @param idGenerator A Callable returning unique session id Strings. + */ + public Factory(Callable<String> idGenerator) { + this.idGenerator = idGenerator; + } + + private synchronized Session createNewSession() { + String id; + try { + id = idGenerator.call(); + } catch (Exception ignored) { + // Most impls aren't going to throw so don't make caller handle a nonexistent checked exception + return null; + } + if (sessionStore.containsKey(id)) { + // Collision, should also never happen + return null; Review Comment: Throw an AssertionError or some other thing to indicate a violated invariant ########## cpp/src/arrow/flight/sql/server_session_middleware_factory.h: ########## @@ -0,0 +1,55 @@ +// 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. + +// ServerSessionMiddlewareFactory, factored into a separate header for testability + +#pragma once + +#include <functional> +#include <map> +#include <memory> +#include <shared_mutex> +#include <utility> +#include <vector> + +namespace arrow { +namespace flight { +namespace sql { + +/// \brief A factory for ServerSessionMiddleware, itself storing session data. +class ServerSessionMiddlewareFactory : public ServerMiddlewareFactory { + protected: + std::map<std::string, std::shared_ptr<FlightSqlSession>> session_store_; + std::shared_mutex session_store_lock_; + std::function<std::string()> id_generator_; + + std::vector<std::pair<std::string, std::string>> ParseCookieString( + const std::string_view& s); + + public: + explicit ServerSessionMiddlewareFactory(std::function<std::string()> id_gen) + : id_generator_(id_gen) {} + Status StartCall(const CallInfo&, const CallHeaders& incoming_headers, + std::shared_ptr<ServerMiddleware>* middleware); Review Comment: ```suggestion std::shared_ptr<ServerMiddleware>* middleware) override; ``` -- 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]
