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

Bring Apache Arrow classes into arrow-spi library so those could be shared between plugins #17580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reta
Copy link
Collaborator

@reta reta commented Mar 13, 2025

Description

Bring Apache Arrow classes into arrow-spi library so those could be shared between plugins

Related Issues

N/A

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.

Copy link
Contributor

❌ Gradle check result for fa8f0d8: 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 e81963a: 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 e81963a: SUCCESS

Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.37%. Comparing base (e306d51) to head (107264e).
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #17580      +/-   ##
============================================
+ Coverage     72.27%   72.37%   +0.09%     
- Complexity    65611    65697      +86     
============================================
  Files          5311     5311              
  Lines        304942   304946       +4     
  Branches      44225    44226       +1     
============================================
+ Hits         220407   220690     +283     
+ Misses        66448    66197     -251     
+ Partials      18087    18059      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

❌ Gradle check result for 107264e: 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 107264e: 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 107264e: 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 107264e: UNSTABLE

  • TEST FAILURES:
      2 org.opensearch.repositories.s3.S3BlobStoreRepositoryTests.classMethod

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@reta reta added the v3.0.0 Issues and PRs related to version 3.0.0 label Mar 15, 2025
@rishabhmaurya
Copy link
Contributor

rishabhmaurya commented Mar 16, 2025

@reta I'm guessing this logic will not work for any code in server module assuming the invariant that server cannot depend on netty (netty-common and netty-buffer in this case)?
Plugins using BufferAllocator and creating VectorSchemaRoot can add netty deps to their classpaths where this change will help?
Thanks for taking this up

@reta
Copy link
Collaborator Author

reta commented Mar 17, 2025

@reta I'm guessing this logic will not work for any code in server module assuming the invariant that server cannot depend on netty (netty-common and netty-buffer in this case)?

@rishabhmaurya AFAIK, there is no direct or transitive dependencies on netty-* in the arrow-spi module (please correct me if I am wrong here).

Plugins using BufferAllocator and creating VectorSchemaRoot can add netty deps to their classpaths where this change will help?

It all depends how the classes will be accessed. The BufferAllocator would be coming from server class loader and shared between others, same for VectorSchemaRoot. If you have a case in mind, please let me know, thank you.

@rishabhmaurya
Copy link
Contributor

rishabhmaurya commented Mar 17, 2025

@reta

It all depends how the classes will be accessed. The BufferAllocator would be coming from server class loader and shared between others, same for VectorSchemaRoot. If you have a case in mind, please let me know, thank you.

You're right! If we choose to use netty based buffer allocator, which is what we are planning to be the default, then arrow-memory-netty(https://mvnrepository.com/artifact/org.apache.arrow/arrow-memory-netty/18.2.0) is required which in turn has a compile time dependency on netty-common and runtime dependency on netty-buffer.

Currently, we are not creating VectorSchemaRoot as part of the integ test (https://github.com/opensearch-project/OpenSearch/blob/main/plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/ArrowFlightServerIT.java#L51), but with #17446, where we have added integration tests creating buffers and vectorSchemaRoot in the plugin, my fear is these might fail because of above reasoning

@rishabhmaurya
Copy link
Contributor

Allocation manager is set here

static final Setting<String> ARROW_ALLOCATION_MANAGER_TYPE = Setting.simpleString(
"arrow.allocation.manager.type",
"Netty",
Setting.Property.NodeScope
);

@reta
Copy link
Collaborator Author

reta commented Mar 18, 2025

If we choose to use netty based buffer allocator, which is what we are planning to be the default, then arrow-memory-netty(https://mvnrepository.com/artifact/org.apache.arrow/arrow-memory-netty/18.2.0) is required which in turn has a compile time dependency on netty-common and runtime dependency on netty-buffer.

I believe as far as the exposed types are backed by arrow-format / arrow-memory / arrow-vector, and referenced as such, we should be fine

@reta
Copy link
Collaborator Author

reta commented Mar 18, 2025

Allocation manager is set here

This is plugin specific setting, should work believe (all other plugins should only see BufferAllocator).

@rishabhmaurya
Copy link
Contributor

@reta please set following system properties in config/jvm.options and enable feature flag - opensearch.experimental.feature.arrow.streams.enabled and run using -

-Dio.netty.allocator.numDirectArenas=1
-Dio.netty.noUnsafe=false
-Dio.netty.tryUnsafe=true
-Dio.netty.tryReflectionSetAccessible=true

./gradlew run -PinstalledPlugins="['arrow-flight-rpc']"

Ideally, it should have been caught as part of internal cluster test, but classpath and dependencies of all tests including internalClusterTests is way different from actual runtime classpath.

This is the error i'm getting -

» ERROR][o.o.b.OpenSearchUncaughtExceptionHandler] [runTask-0] fatal error in thread [main], exiting
»  java.lang.NoClassDefFoundError: org/slf4j/LoggerFactory
»       at org.apache.arrow.memory.BaseAllocator.<clinit>(BaseAllocator.java:52) ~[arrow-memory-core-18.2.0.jar:18.2.0]
»       at org.opensearch.arrow.flight.bootstrap.FlightService.lambda$doStart$0(FlightService.java:91) ~[?:?]
»       at java.base/java.security.AccessController.doPrivileged(AccessController.java:319) ~[?:?]
»       at org.opensearch.arrow.flight.bootstrap.FlightService.doStart(FlightService.java:91) ~[?:?]
»       at org.opensearch.common.lifecycle.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:77) ~[opensearch-common-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at java.base/java.util.ArrayList.forEach(ArrayList.java:1597) ~[?:?]
»       at java.base/java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1117) ~[?:?]
»       at org.opensearch.node.Node.start(Node.java:1637) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.Bootstrap.start(Bootstrap.java:340) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:414) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:181) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:172) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.common.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:110) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138) ~[opensearch-cli-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.cli.Command.main(Command.java:101) ~[opensearch-cli-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:138) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»       at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:104) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  Caused by: java.lang.ClassNotFoundException: org.slf4j.LoggerFactory
»       at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) ~[?:?]
»       at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) ~[?:?]
»       at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:528) ~[?:?]
»       ... 17 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants