Skip to content
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

Arrow Flight Server bootstrap logic #16962

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

rishabhmaurya
Copy link
Contributor

@rishabhmaurya rishabhmaurya commented Jan 6, 2025

Description

This PR adds server bootstrap logic for Arrow Flight. This is in continuation to #16691

  • Flight server bootstrap logic and integration with Auxiliary Transport.
  • new plugin for StreamManager implementation behind feature flag with default behaviour as disabled.
  • support for SslContext in Flight server and client.
  • update in SecureTransportSettingsProvider to build SslContext outside of security plugin.
  • ClientManager for creating a pool of flight clients for data nodes only.
  • custom event loop group and thread pool for server and client channel.
  • Default netty server and allocator configuration defined in ServerConfig.
  • 2 new gradle projects - arrow-memory-shaded and flight-core-shaded to share dependencies to avoid polluting server classpath.
  • Flight info API to get the publish and bound address for each of the node.
  • Integration test for FlightServer client handshake.
  • StreamManager wrapper for integration with task API. moved to next PR (Base Flight producer integration with Flight Server #17066)
  • integration of StreamManager with server module. moved to next PR (Base Flight producer integration with Flight Server #17066)

TODOs:

I will handle both of them as part of follow up PRs.

Related Issues

Resolves #16963

#16679

Check List

  • Functionality includes testing.
    - [ ] API changes companion pull request created, if applicable.
    - [ ] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions github-actions bot added enhancement Enhancement or improvement to existing feature or request Search:Performance labels Jan 6, 2025
Copy link
Contributor

github-actions bot commented Jan 6, 2025

❌ Gradle check result for 53ad56f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@rishabhmaurya rishabhmaurya self-assigned this Jan 6, 2025
Copy link
Contributor

github-actions bot commented Jan 6, 2025

❌ Gradle check result for f360c80: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@rishabhmaurya rishabhmaurya force-pushed the stream-bootstrap branch 2 times, most recently from 83b63d3 to b38f302 Compare January 6, 2025 20:52
Copy link
Contributor

github-actions bot commented Jan 6, 2025

❌ Gradle check result for b38f302: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta
Copy link
Collaborator

reta commented Jan 6, 2025

@rishabhmaurya I know that this is work in progress but please take a look at [1] where the notion of auxiliary transports (and plugins implementing ones, like gRPC), has been introduced. We may (and very likely - should) consider Apache Arrow Flight as one of such transports, it should be optional plugin and not part of the core server. Thank you

[1] #16534

@rishabhmaurya
Copy link
Contributor Author

@reta thanks for the pointer, I will update the PR and try to use aux transport.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

❌ Gradle check result for 60f82e9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jan 7, 2025

❌ Gradle check result for f9623e9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@rishabhmaurya rishabhmaurya force-pushed the stream-bootstrap branch 2 times, most recently from 4601334 to 93df0aa Compare January 7, 2025 01:03
Copy link
Contributor

github-actions bot commented Jan 7, 2025

❌ Gradle check result for 93df0aa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jan 7, 2025

❌ Gradle check result for 63bc0f4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jan 7, 2025

❌ Gradle check result for d364c1c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@rishabhmaurya
Copy link
Contributor Author

rishabhmaurya commented Jan 7, 2025

@reta do you know how can i address the gradle check failure related to ApiAnnotationProcessorTests ? I know i have messed something up but unable to troubleshoot.

java.lang.AssertionError: 
Expected: an instance of org.opensearch.common.annotation.processor.CompilerSupport$Failure
     but: <org.opensearch.common.annotation.processor.CompilerSupport$Success@1b7367d4> is a org.opensearch.common.annotation.processor.CompilerSupport$Success
	at __randomizedtesting.SeedInfo.seed([188B34A9BBC5FD49:D29516146CC3C7B0]:0)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.junit.Assert.assertThat(Assert.java:964)
	at org.junit.Assert.assertThat(Assert.java:930)
	at org.opensearch.common.annotation.processor.ApiAnnotationProcessorTests.testPublicApiMethodReturnNotAnnotatedWildcardGenerics(ApiAnnotationProcessorTests.java:310)

ref: https://build.ci.opensearch.org/job/gradle-check/51929/testReport/junit/org.opensearch.common.annotation.processor/ApiAnnotationProcessorTests/testPublicApiMethodReturnNotAnnotatedWildcardGenerics/

@reta
Copy link
Collaborator

reta commented Jan 7, 2025

@reta do you know how can i address the gradle check failure related to ApiAnnotationProcessorTests ? I know i have messed something up but unable to troubleshoot.

@rishabhmaurya It is caused by https://github.com/opensearch-project/OpenSearch/pull/16962/files#diff-756d8dcb1dfd9f8fa9bf974880af0b001cef12541eb17857ff30d4aa922bc05eR200 (change in server/src/main/resources/org/opensearch/bootstrap/security.policy

@rishabhmaurya
Copy link
Contributor Author

@reta ah, thanks. How did you find that out as I couldn't find any error message in the logs

@reta
Copy link
Collaborator

reta commented Jan 7, 2025

@reta ah, thanks. How did you find that out as I couldn't find any error message in the logs

@rishabhmaurya needed some debugging, afair some tests were required some more security permissions but those could be ignored (since tests are focused on diagnostics)

@rishabhmaurya rishabhmaurya removed the v2.19.0 Issues and PRs related to version 2.19.0 label Jan 28, 2025
api "com.fasterxml.jackson.core:jackson-annotations:${versions.jackson}"

implementation "commons-codec:commons-codec:${versions.commonscodec}"
api project(path:':libs:opensearch-arrow-memory-shaded', configuration: 'shadow')
Copy link
Collaborator

@reta reta Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these are the only dependencies that SPI layer needs:

dependencies {
  api project(':libs:opensearch-core')
  api "org.apache.arrow:arrow-memory-core:${versions.arrow}"
  api "org.apache.arrow:arrow-vector:${versions.arrow}"
}

Copy link
Contributor Author

@rishabhmaurya rishabhmaurya Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But at runtime, there are other transitive dependencies needed like netty-buffer, jackson-annotations etc which are part of arrow-memory-netty-shaded as shaded dependencies.
So if we have to use vector in server module, then these runtime dependencies should be present on its runtime classpath.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we have to use vector in server module, then these runtime dependencies should be present on its runtime classpath.

We will not use vector in server module, the server has to be sufficient with SPI layer at the moment (since everything else is in plugin)


dependencies {
implementation project(':libs:opensearch-arrow-spi')
implementation project(path:':libs:opensearch-flight-core-shaded', configuration: 'shadow')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin could bundle all these dependencies directly, right? There is no need to shade as far as we don't have netty / jackson JarHell (that we shouldn't)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugin has a different classloader than server, so even if plugin adds them to their classloader, it won't work for server at runtime.

* new plugin for StreamManager implementation
* integration with server module
* support for SslContext in Flight server and client
* ClientManager for creating a pool of flight clients for data nodes
* custom event loop group and thread pool for server and client channel

Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
…on was causing client close failure

Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Signed-off-by: Rishabh Maurya <[email protected]>
Copy link
Contributor

❌ Gradle check result for 126f8f0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for afea399: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Search:Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Arrow Flight server and client bootstrap logic
4 participants