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

Draft
wants to merge 2 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

  • new plugin for StreamManager implementation behind feature flag with default behaviour as disabled.
  • integration of StreamManager with server module.
  • 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.
  • StreamManager wrapper for integration with task API.

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?

* 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]>
@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)

dependencies {
implementation project(':libs:opensearch-arrow-spi')

implementation "io.netty:netty-buffer:${versions.netty}"
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using the netty shaded gRPC dependency? The gRPC maintainers seem to recommend using that as they sometimes depend on unstable Netty APIs and the shaded version avoids problems on upgrades.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, let me try

Copy link
Contributor

github-actions bot commented Jan 9, 2025

❌ Gradle check result for cde986a: 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
3 participants