ARROW-10467: [FlightRPC][Java] Add the ability to pass arbitrary client headers. by kylepbit · Pull Request #8572 · apache/arrow

kylepbit · 2020-11-02T18:03:49Z

PR for https://issues.apache.org/jira/browse/ARROW-10467

Add a CallOption for specifying arbitrary properties as headers for RPC calls.
Add a ServerMiddleware that will intercept and pass the properties to a handler
when registered via the FlightServer builder.

client properties.

github-actions · 2020-11-02T18:04:45Z

https://issues.apache.org/jira/browse/ARROW-10467

lidavidm · 2020-11-02T20:48:35Z

What do you think about instead exposing headers as key-value metadata directly (on the sending side with a CallOption, on the receiving side via CallContext)? You could even still implement the server side like you've done here, with a standard middleware that can be configured.

lidavidm · 2020-11-02T20:51:43Z

Ah, I see you want some keys that wouldn't necessarily be valid headers.

lidavidm · 2020-11-02T20:52:02Z

Just stepping back for some context - what's the use case you have in mind for this?

kylepbit · 2020-11-02T22:03:18Z

The intent is to be able to provide preferences to requests that would not be part of the actual action. Ex context to run the action in, etc.

lidavidm · 2020-11-03T15:42:11Z

Ok. I think it'd make more sense to implement better support for headers in general, rather than have an interface to a single hardcoded header in Flight - anything higher-level (such as encoding an options map) can then be implemented in the application. Does that sound reasonable?

kylepbit · 2020-11-03T16:47:55Z

Yes - do you have suggestions as to how you think that would be best done? Perhaps a CallOption to specify a header and give it a key and value where the value is serializable, and have a method that serializes the value that can be overridden if people wish to specialize? The server could register a set of headers that it would like to receive.

tifflhl · 2020-11-04T00:44:45Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java

+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer<CallHeaders> {


Should we add a unit test that demonstrate a sample usage for this handler?

There already is: see https://github.com/apache/arrow/pull/8572/files#diff-b19522af7605b7cfe033ecc1300f2f6b0347d7eaba2969f0193086eb9751a546R102

tifflhl · 2020-11-04T00:47:17Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderMiddleware.java

+/**
+ * Middleware that's used to extract and pass properties to the server during requests.
+ */
+public class ServerHeaderMiddleware implements FlightServerMiddleware {


Can we rename ServerHeaderMiddleware to ServerPropertyMiddleware? The current name is a bit too generic.

I think the name is appropriate given that this is intended to handle all headers for calls, which could be considered properties depending on the FlightServer implementation, but not necessarily.

tifflhl · 2020-11-04T00:47:49Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java

+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer<CallHeaders> {


Can we rename ServerHeaderHandler to ServerPropertyHandler? Like ServerHeaderMiddleware, the current name is a too generic.

I agree with @tifflhl's suggestion the name is too generic, would something like ServerPropertyHeaderHandler and ClientPropertyHeaderHandler be more appropriate similar to ClientBearerHeaderHandler.

This is not specific to properties - it's generally for handling all headers so I think the name fits.

lidavidm · 2020-11-04T01:50:30Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightConstants.java

 public interface FlightConstants {

  String SERVICE = "arrow.flight.protocol.FlightService";
+  String PROPERTY_HEADER = "Arrow-Properties";


Let's rename this to something more specific (HEADERS_KEY?) and make it an instance of FlightServerMiddleware.Key. Let's also change the constant to something that's more clearly namespaced under arrow, e.g. "org.apache.arrow.flight.ServerHeaderHandler".

lidavidm · 2020-11-04T01:52:38Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java

+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer<CallHeaders> {


This interface confuses me, as it seems within an RPC handler, there's no way to get the headers associated with that particular call.

Not sure I understand the comment, are you saying you can't correlate the headers that are incoming with the RPC call that generated them?

Yes - within the body of an RPC handler, how do I access the headers that the client sent for that call?

lidavidm · 2020-11-04T01:53:55Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestCallOptions.java


+  @Test
+  public void singleProperty() {
+    final MetadataAdapter headers = new MetadataAdapter(new Metadata());


We probably need a CallHeaders implementation that doesn't come from the flight-grpc package, or there's no way to use HeaderCallOption.

keeratsingh · 2020-11-04T01:41:34Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java

+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer<CallHeaders> {


I agree with @tifflhl's suggestion the name is too generic, would something like ServerPropertyHeaderHandler and ClientPropertyHeaderHandler be more appropriate similar to ClientBearerHeaderHandler.

keeratsingh · 2020-11-04T01:42:30Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java

+    /**
+     * Set the header handler.
+     */
+    public Builder headerHandler(ServerHeaderHandler headerHandler) {


Can we rename this to propertyHeaderHandler if we decide to rename ServerHeaderHandler
to ServerPropertyHeaderHandler?

As mentioned in another comment, the intent is to handle all headers. You can interpret them as properties, but I think the name is reflective of intent.

lidavidm · 2020-11-04T23:03:31Z

Just a higher level comment - I see on ARROW-10428 cookie support is being worked on and ARROW-10427 is about session headers. Since cookies are a particular header and sessions are (usually) a particular cookie, is there any interest in making sure all these features work together and are built on top of each other?

headers in FlightClient. Change how FlightServer accesses headers by requesting the ServerHeaderMiddleware via the CallContext in the RPC handlers.

kylepbit · 2020-11-05T06:28:11Z

Regarding the comment about sessions and cookies - yes, there is definite interest in making sure that the work there is building on top of, instead of parallel to, one another.

kylepbit · 2020-11-05T22:58:48Z

@lidavidm any other comments on this?

lidavidm

Overall looks good. Two minor comments.

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightCallHeaders.java

java/flight/flight-core/src/main/java/org/apache/arrow/flight/HeaderCallOption.java

Fix bug trimming values from multi-valued headers when specifying as a HeaderCallOption.

lidavidm · 2020-11-06T13:49:36Z

Thanks @kylepbit!

…nt headers. PR for https://issues.apache.org/jira/browse/ARROW-10467 Add a CallOption for specifying arbitrary properties as headers for RPC calls. Add a ServerMiddleware that will intercept and pass the properties to a handler when registered via the FlightServer builder. Closes apache#8572 from kylepbit/ARROW-10467 Authored-by: Kyle Porter <[email protected]> Signed-off-by: David Li <[email protected]>

ARROW-10467: [Java] Add the ability to pass arbitrary

c65142c

client properties.

Change design to allow specification and reception of custom headers.

c2f61a6

kylepbit changed the title ~~ARROW-10467: [Java] Add the ability to pass arbitrary client properties.~~ ARROW-10467: [Java] Add the ability to pass arbitrary client headers. Nov 4, 2020

tifflhl reviewed Nov 4, 2020

View reviewed changes

lidavidm reviewed Nov 4, 2020

View reviewed changes

keeratsingh reviewed Nov 4, 2020

View reviewed changes

Add a Flight implementation of CallHeaders, for use when specifying

ba63089

headers in FlightClient. Change how FlightServer accesses headers by requesting the ServerHeaderMiddleware via the CallContext in the RPC handlers.

lidavidm changed the title ~~ARROW-10467: [Java] Add the ability to pass arbitrary client headers.~~ ARROW-10467: [FlightRPC][Java] Add the ability to pass arbitrary client headers. Nov 5, 2020

lidavidm mentioned this pull request Nov 5, 2020

ARROW-10428: [FlightRPC][Java] Add support for HTTP cookies #8554

Closed

lidavidm reviewed Nov 6, 2020

View reviewed changes

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightCallHeaders.java Outdated Show resolved Hide resolved

java/flight/flight-core/src/main/java/org/apache/arrow/flight/HeaderCallOption.java Outdated Show resolved Hide resolved

Make FlightCallHeaders return null if specific header is not present.

3600b12

Fix bug trimming values from multi-valued headers when specifying as a HeaderCallOption.

lidavidm approved these changes Nov 6, 2020

View reviewed changes

lidavidm closed this in b26478a Nov 6, 2020

asfimport mentioned this pull request Nov 6, 2020

[FlightRPC][Java] Ability to pass arbitrary client properties to server #26443

Closed

Conversation

kylepbit commented Nov 2, 2020

Uh oh!

github-actions bot commented Nov 2, 2020

Uh oh!

lidavidm commented Nov 2, 2020

Uh oh!

lidavidm commented Nov 2, 2020

Uh oh!

lidavidm commented Nov 2, 2020

Uh oh!

kylepbit commented Nov 2, 2020

Uh oh!

lidavidm commented Nov 3, 2020

Uh oh!

kylepbit commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylepbit Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Nov 4, 2020

Uh oh!

kylepbit commented Nov 5, 2020

Uh oh!

kylepbit commented Nov 5, 2020

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lidavidm commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kylepbit commented Nov 3, 2020 •

edited

Loading

kylepbit Nov 4, 2020 •

edited

Loading