-
Notifications
You must be signed in to change notification settings - Fork 4.2k
ARROW-10467: [FlightRPC][Java] Add the ability to pass arbitrary client headers. #8572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
c65142c
c2f61a6
ba63089
3600b12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,6 +168,7 @@ public static final class Builder { | |
| private final Map<String, Object> builderOptions; | ||
| private ServerAuthHandler authHandler = ServerAuthHandler.NO_OP; | ||
| private CallHeaderAuthenticator headerAuthenticator = CallHeaderAuthenticator.NO_OP; | ||
| private ServerHeaderHandler headerHandler = ServerHeaderHandler.NO_OP; | ||
| private ExecutorService executor = null; | ||
| private int maxInboundMessageSize = MAX_GRPC_MESSAGE_SIZE; | ||
| private InputStream certChain; | ||
|
|
@@ -196,6 +197,13 @@ public FlightServer build() { | |
| this.middleware(FlightServerMiddleware.Key.of(Auth2Constants.AUTHORIZATION_HEADER), | ||
| new ServerCallHeaderAuthMiddleware.Factory(headerAuthenticator)); | ||
| } | ||
|
|
||
| // Add the property handler middleware if applicable. | ||
| if (headerHandler != ServerHeaderHandler.NO_OP) { | ||
| this.middleware(FlightServerMiddleware.Key.of(FlightConstants.PROPERTY_HEADER), | ||
| new ServerHeaderMiddleware.Factory(headerHandler)); | ||
| } | ||
|
|
||
| final NettyServerBuilder builder; | ||
| switch (location.getUri().getScheme()) { | ||
| case LocationSchemes.GRPC_DOMAIN_SOCKET: { | ||
|
|
@@ -351,6 +359,14 @@ public Builder headerAuthenticator(CallHeaderAuthenticator headerAuthenticator) | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Set the header handler. | ||
| */ | ||
| public Builder headerHandler(ServerHeaderHandler headerHandler) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename this to propertyHeaderHandler if we decide to rename
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| this.headerHandler = headerHandler; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Provide a transport-specific option. Not guaranteed to have any effect. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /* | ||
| * 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 io.grpc.Metadata; | ||
| import io.grpc.stub.AbstractStub; | ||
| import io.grpc.stub.MetadataUtils; | ||
|
|
||
| /** | ||
| * Method option for supplying headers to method calls. | ||
| */ | ||
| public class HeaderCallOption implements CallOptions.GrpcCallOption { | ||
| private final Metadata propertiesMetadata = new Metadata(); | ||
|
|
||
| /** | ||
| * Header property constructor. | ||
| * | ||
| * @param headers the headers that should be sent across. If a header is a string, it should only be valid ASCII | ||
| * characters. Binary headers should end in "-bin". | ||
| */ | ||
| public HeaderCallOption(CallHeaders headers) { | ||
| for (String key : headers.keys()) { | ||
| if (key.endsWith(Metadata.BINARY_HEADER_SUFFIX)) { | ||
| propertiesMetadata.put( | ||
|
lidavidm marked this conversation as resolved.
Outdated
|
||
| Metadata.Key.of(key, Metadata.BINARY_BYTE_MARSHALLER), | ||
| headers.getByte(key)); | ||
| } else { | ||
| propertiesMetadata.put( | ||
| Metadata.Key.of(key, Metadata.ASCII_STRING_MARSHALLER), | ||
| headers.get(key)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public <T extends AbstractStub<T>> T wrapStub(T stub) { | ||
| return MetadataUtils.attachHeaders(stub, propertiesMetadata); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /* | ||
| * 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.function.Consumer; | ||
|
|
||
| /** | ||
| * Interface for server side property handling. | ||
| */ | ||
| public interface ServerHeaderHandler extends Consumer<CallHeaders> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a unit test that demonstrate a sample usage for this handler?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename ServerHeaderHandler to ServerPropertyHandler? Like ServerHeaderMiddleware, the current name is a too generic.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @tifflhl's suggestion the name is too generic, would something like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not specific to properties - it's generally for handling all headers so I think the name fits.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This interface confuses me, as it seems within an RPC handler, there's no way to get the headers associated with that particular call.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - within the body of an RPC handler, how do I access the headers that the client sent for that call? |
||
| /** | ||
| * A property handler that does nothing. | ||
| */ | ||
| ServerHeaderHandler NO_OP = (s) -> { }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| /** | ||
| * Middleware that's used to extract and pass properties to the server during requests. | ||
| */ | ||
| public class ServerHeaderMiddleware implements FlightServerMiddleware { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename ServerHeaderMiddleware to ServerPropertyMiddleware? The current name is a bit too generic.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| /** | ||
| * Factory for accessing ServerCallPropertyMiddleware. | ||
| */ | ||
| public static class Factory implements FlightServerMiddleware.Factory<ServerHeaderMiddleware> { | ||
| private final ServerHeaderHandler propertyHandler; | ||
|
|
||
| /** | ||
| * Construct a factory with the given header handler. | ||
| * @param headerHandler The header handler that will be used to pass properties. | ||
| */ | ||
| public Factory(ServerHeaderHandler headerHandler) { | ||
| this.propertyHandler = headerHandler; | ||
| } | ||
|
|
||
| @Override | ||
| public ServerHeaderMiddleware onCallStarted(CallInfo callInfo, CallHeaders incomingHeaders, | ||
| RequestContext context) { | ||
| propertyHandler.accept(incomingHeaders); | ||
| return new ServerHeaderMiddleware(); | ||
| } | ||
| } | ||
|
|
||
| private ServerHeaderMiddleware() { | ||
| } | ||
|
|
||
| @Override | ||
| public void onBeforeSendingHeaders(CallHeaders outgoingHeaders) { | ||
| } | ||
|
|
||
| @Override | ||
| public void onCallCompleted(CallStatus status) { | ||
| } | ||
|
|
||
| @Override | ||
| public void onCallErrored(Throwable err) { | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,12 +24,15 @@ | |
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.Consumer; | ||
|
|
||
| import org.apache.arrow.flight.grpc.MetadataAdapter; | ||
| import org.apache.arrow.memory.BufferAllocator; | ||
| import org.apache.arrow.memory.RootAllocator; | ||
| import org.junit.Assert; | ||
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
|
|
||
| import io.grpc.Metadata; | ||
|
|
||
| public class TestCallOptions { | ||
|
|
||
| @Test | ||
|
|
@@ -64,6 +67,61 @@ public void underTimeout() { | |
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void singleProperty() { | ||
| final MetadataAdapter headers = new MetadataAdapter(new Metadata()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need a CallHeaders implementation that doesn't come from the flight-grpc package, or there's no way to use HeaderCallOption. |
||
| headers.insert("key", "value"); | ||
| testHeaders(headers); | ||
| } | ||
|
|
||
| @Test | ||
| public void multipleProperties() { | ||
| final MetadataAdapter headers = new MetadataAdapter(new Metadata()); | ||
| headers.insert("key", "value"); | ||
| headers.insert("key2", "value2"); | ||
| testHeaders(headers); | ||
| } | ||
|
|
||
| @Test | ||
| public void binaryProperties() { | ||
| final MetadataAdapter headers = new MetadataAdapter(new Metadata()); | ||
| headers.insert("key-bin", "value".getBytes()); | ||
| headers.insert("key3-bin", "ëfßæ".getBytes()); | ||
| testHeaders(headers); | ||
| } | ||
|
|
||
| @Test | ||
| public void mixedProperties() { | ||
| final MetadataAdapter headers = new MetadataAdapter(new Metadata()); | ||
| headers.insert("key", "value"); | ||
| headers.insert("key3-bin", "ëfßæ".getBytes()); | ||
| testHeaders(headers); | ||
| } | ||
|
|
||
| private void testHeaders(MetadataAdapter headers) { | ||
| final ServerHeaderHandler propertyHandler = (incomingHeaders) -> { | ||
| for (String key : headers.keys()) { | ||
| if (key.endsWith(Metadata.BINARY_HEADER_SUFFIX)) { | ||
| Assert.assertArrayEquals(headers.getByte(key), incomingHeaders.getByte(key)); | ||
| } else { | ||
| Assert.assertEquals(headers.get(key), incomingHeaders.get(key)); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| try ( | ||
| BufferAllocator a = new RootAllocator(Long.MAX_VALUE); | ||
| Producer producer = new Producer(a); | ||
| FlightServer s = | ||
| FlightTestUtil.getStartedServer((location) -> | ||
| FlightServer.builder(a, location, producer).headerHandler(propertyHandler).build()); | ||
| FlightClient client = FlightClient.builder(a, s.getLocation()).build()) { | ||
| client.doAction(new Action("fast"), new HeaderCallOption(headers)).hasNext(); | ||
| } catch (InterruptedException | IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| void test(Consumer<FlightClient> testFn) { | ||
| try ( | ||
| BufferAllocator a = new RootAllocator(Long.MAX_VALUE); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".