Skip to content

Add AnalyticsFrontEndExtension SPI + producer-side wiring in AnalyticsPlugin - #21453

Closed
ahkcs wants to merge 1 commit into
opensearch-project:mainfrom
ahkcs:feature/analytics-spi-e2e
Closed

Add AnalyticsFrontEndExtension SPI + producer-side wiring in AnalyticsPlugin#21453
ahkcs wants to merge 1 commit into
opensearch-project:mainfrom
ahkcs:feature/analytics-spi-e2e

Conversation

@ahkcs

@ahkcs ahkcs commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an SPI in analytics-framework that lets frontend plugins (e.g., opensearch-sql) consume analytics-engine services without taking a hard install-time dependency on analytics-engine. Adds the matching producer-side wiring in AnalyticsPlugin: discover consumers via ExtensiblePlugin#loadExtensions, push the executor + schemaProvider once Guice constructs DefaultPlanExecutor.

Mirrors the JobSchedulerExtension pattern from opensearch-job-scheduler.

Why

Today opensearch-sql declares extendedPlugins = [..., 'analytics-engine'] as a HARD dependency. SQL plugin install fails on stock OpenSearch distros that don't ship analytics-engine (Missing plugin [analytics-engine]). Marking it ;optional=true is necessary but not sufficient — TransportPPLQueryAction Guice-injects QueryPlanExecutor, and Guice cannot satisfy that binding when the providing plugin is absent.

The SPI inverts the dependency. Frontend plugins implement AnalyticsFrontEndExtension to receive analytics-engine's services through a push lifecycle; analytics-engine never imports the frontend.

What's in this PR

sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/:

  • AnalyticsFrontEndExtension — single-method SPI: setAnalyticsServices(AnalyticsServices). Implementers declared via standard Java SPI (META-INF/services).
  • AnalyticsServices — record bundling QueryPlanExecutor<RelNode, Iterable<Object[]>> and SchemaProvider. Bundled (rather than separate setters) so future analytics-engine services can be added without changing the SPI signature.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/AnalyticsPlugin.java:

  • loadExtensions(loader) — collects AnalyticsFrontEndExtension consumers alongside the existing AnalyticsSearchBackendPlugin collection.
  • createGuiceModules — adds a Guice TypeListener that fires when DefaultPlanExecutor is instantiated. The InjectionListener wraps OpenSearchSchemaBuilder::buildSchema as a SchemaProvider, packs both into AnalyticsServices, and pushes to every registered consumer.
  • pushAnalyticsServices(...) — idempotent (guarded by servicesPushed flag) so Guice re-instantiation doesn't re-fire the callback. Per-consumer try/catch so one bad consumer doesn't crash the others.

End-to-end verification

Built this branch + the matching consumer-side PR (opensearch-project/sql#5xxx), assembled an OpenSearch 3.7 distro, and ran bin/opensearch-plugin install followed by real PPL queries against a single-node cluster.

Scenario A — analytics-engine INSTALLED

$ bin/opensearch-plugin install --batch file:///.../opensearch-job-scheduler-3.7.0.0-SNAPSHOT.zip
-> Installed opensearch-job-scheduler with folder name opensearch-job-scheduler

$ bin/opensearch-plugin install --batch file:///.../analytics-engine-3.7.0-SNAPSHOT.zip
-> Installed analytics-engine with folder name analytics-engine

$ bin/opensearch-plugin install --batch file:///.../opensearch-sql-3.7.0.0-SNAPSHOT.zip
-> Installed opensearch-sql with folder name opensearch-sql

Node started cleanly:

$ curl -s 'http://localhost:19201/_cat/plugins?v'
name         component                version
6c7e67cac848 analytics-engine         3.7.0-SNAPSHOT
6c7e67cac848 opensearch-job-scheduler 3.7.0.0-SNAPSHOT
6c7e67cac848 opensearch-sql           3.7.0.0-SNAPSHOT

Lucene PPL query against a regular index:

$ curl -s -X POST 'http://localhost:19201/_plugins/_ppl' -H 'Content-Type: application/json' \
       -d '{"query":"source = accounts | fields name, age"}'
{
  "schema":[{"name":"name","type":"string"},{"name":"age","type":"int"}],
  "datarows":[["Alice",30]],
  "total":1, "size":1
}

Parquet-prefixed PPL query (routes through analytics-engine — confirms the SPI fired and the executor was received):

$ curl -s -X POST 'http://localhost:19201/_plugins/_ppl/_explain' -H 'Content-Type: application/json' \
       -d '{"query":"source = parquet_logs | head 1"}'
{
  "calcite":{
    "logical":"LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])\n  LogicalSort(fetch=[1])\n    LogicalTableScan(table=[[opensearch, parquet_logs]])\n"
  }
}

The calcite.logical block is produced only by the analytics-engine planner — it confirms the routing went through RestUnifiedQueryAction → AnalyticsExecutionEngine, which proves the SPI push lifecycle worked end-to-end (consumer received services, holder was populated, lazy handler was constructed, request was dispatched).

Scenario B — analytics-engine NOT installed (the optional path)

$ bin/opensearch-plugin install --batch file:///.../opensearch-job-scheduler-3.7.0.0-SNAPSHOT.zip
-> Installed opensearch-job-scheduler with folder name opensearch-job-scheduler

$ bin/opensearch-plugin install --batch file:///.../opensearch-sql-3.7.0.0-SNAPSHOT.zip
2026-05-01T... main WARN Missing plugin [analytics-engine], dependency of [opensearch-sql]
-> Installed opensearch-sql with folder name opensearch-sql

Note the WARN (not ERROR) for the missing optional dep — that's the ;optional=true mechanism in PluginsService.checkBundleJarHell working as intended.

Node started cleanly with only two plugins:

$ curl -s 'http://localhost:19200/_cat/plugins?v'
name         component                version
6c7e67cac848 opensearch-job-scheduler 3.7.0.0-SNAPSHOT
6c7e67cac848 opensearch-sql           3.7.0.0-SNAPSHOT

Lucene PPL query (the legacy path) — works:

$ curl -s -X POST 'http://localhost:19200/_plugins/_ppl' -H 'Content-Type: application/json' \
       -d '{"query":"source = accounts | fields name, age"}'
{
  "schema":[{"name":"name","type":"string"},{"name":"age","type":"int"}],
  "datarows":[["Alice",30],["Bob",25]],
  "total":2, "size":2
}

Parquet-prefixed PPL query — falls through to legacy (no SPI push ever fired, holder stays null, analyticsHandler() returns null, request takes the legacy path):

$ curl -s -X POST 'http://localhost:19200/_plugins/_ppl' -H 'Content-Type: application/json' \
       -d '{"query":"source = parquet_logs | head 1"}'
{
  "error":{
    "reason":"Error occurred in OpenSearch engine: no such index [parquet_logs]",
    "type":"IndexNotFoundException"
  },
  "status":404
}

This is the critical assertion: a clean IndexNotFoundException from the legacy engine — NOT NoClassDefFoundError: org/opensearch/analytics/exec/QueryPlanExecutor (the failure mode this SPI is designed to prevent). The SQL plugin's bytecode never resolves an analytics-framework type when analytics-engine is absent.

Reproducing locally

The full reproduction is in the PR body: build this branch + the SQL PR, then for each scenario:

DIST=opensearch-min-3.7.0-SNAPSHOT-darwin-arm64.tar.gz
tar xzf $DIST -C /tmp/test
cd /tmp/test/opensearch-3.7.0-SNAPSHOT
bin/opensearch-plugin install --batch file://.../opensearch-job-scheduler-3.7.0.0-SNAPSHOT.zip
# Scenario A only:
bin/opensearch-plugin install --batch file://.../analytics-engine-3.7.0-SNAPSHOT.zip
bin/opensearch-plugin install --batch file://.../opensearch-sql-3.7.0.0-SNAPSHOT.zip
OPENSEARCH_JAVA_HOME=$(/usr/libexec/java_home) bin/opensearch -E discovery.type=single-node ...

Pairs with

opensearch-project/sql#5xxx — the SQL-side consumer wiring, must merge after this so it can vendor the rebuilt analytics-framework-3.7.0-SNAPSHOT.jar.

…sPlugin

Adds the SPI surface in analytics-framework that lets frontend plugins
(e.g., opensearch-sql) consume analytics-engine services without taking
a hard install-time dependency on analytics-engine. Adds the
producer-side wiring in AnalyticsPlugin: discover consumers via
ExtensiblePlugin#loadExtensions, push the executor + schemaProvider
bundle once Guice constructs DefaultPlanExecutor.

Mirrors the JobSchedulerExtension pattern from opensearch-job-scheduler.

analytics-framework (sandbox/libs/analytics-framework):
- AnalyticsFrontEndExtension — single-method SPI interface,
  setAnalyticsServices(AnalyticsServices). Implementers declared
  via standard Java SPI (META-INF/services).
- AnalyticsServices — record bundling QueryPlanExecutor and
  SchemaProvider. Bundled rather than separate setters so future
  services can be added without changing the SPI signature.

analytics-engine (sandbox/plugins/analytics-engine):
- AnalyticsPlugin#loadExtensions: collect AnalyticsFrontEndExtension
  consumers alongside the existing AnalyticsSearchBackendPlugin
  collection.
- AnalyticsPlugin#createGuiceModules: bind a Guice TypeListener that
  fires when DefaultPlanExecutor is instantiated. The
  InjectionListener wraps OpenSearchSchemaBuilder::buildSchema as a
  SchemaProvider, packs both into AnalyticsServices, and pushes to
  every registered consumer (per-consumer try/catch so one bad
  consumer doesn't crash the others).
- The push is idempotent — guarded by a servicesPushed flag so Guice
  re-instantiation (no Singleton scope today) doesn't re-fire the
  callback.

Verified end-to-end against opensearch-sql:
- WITH analytics-engine: SQL routes parquet_* queries through the
  pushed executor (confirmed via /_plugins/_ppl/_explain showing
  Calcite logical plan).
- WITHOUT analytics-engine: SQL plugin loads fine; parquet_* queries
  fall through to the legacy path with a clean IndexNotFoundException
  — no NoClassDefFoundError, no startup crash.

Pairs with the SQL-side consumer wiring in opensearch-project/sql.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add AnalyticsFrontEndExtension SPI and AnalyticsServices record to analytics-framework

Relevant files:

  • sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/AnalyticsFrontEndExtension.java
  • sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/AnalyticsServices.java

Sub-PR theme: Wire AnalyticsFrontEndExtension consumer discovery and service push in AnalyticsPlugin

Relevant files:

  • sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/AnalyticsPlugin.java

⚡ Recommended focus areas for review

Race Condition

servicesPushed is a non-volatile boolean field read and written inside a synchronized method, but the field itself is not declared volatile. While the synchronized keyword provides mutual exclusion, if pushAnalyticsServices is ever called from a context where the lock is not held (e.g., future refactoring), the non-volatile read could be stale. Additionally, frontEnds is populated in loadExtensions and read in pushAnalyticsServices without synchronization — if these are called from different threads, there is a potential visibility issue.

private boolean servicesPushed = false;

private synchronized void pushAnalyticsServices(DefaultPlanExecutor executor) {
    if (servicesPushed) {
        return;
    }
    servicesPushed = true;
    SchemaProvider schemaProvider = clusterState -> OpenSearchSchemaBuilder.buildSchema((ClusterState) clusterState);
    AnalyticsServices services = new AnalyticsServices(executor, schemaProvider);
    for (AnalyticsFrontEndExtension consumer : frontEnds) {
        try {
            consumer.setAnalyticsServices(services);
        } catch (Exception e) {
            logger.warn("AnalyticsFrontEndExtension {} threw on setAnalyticsServices", consumer.getClass().getName(), e);
        }
    }
}
Unsafe Cast

In pushAnalyticsServices, the SchemaProvider lambda casts clusterState to ClusterState unconditionally: (ClusterState) clusterState. If SchemaProvider accepts a more generic type and a non-ClusterState argument is ever passed, this will throw a ClassCastException at runtime. The parameter type of the lambda should be verified against the SchemaProvider interface signature.

SchemaProvider schemaProvider = clusterState -> OpenSearchSchemaBuilder.buildSchema((ClusterState) clusterState);
Broad TypeListener

The TypeListener registered with Matchers.any() will be invoked for every type Guice constructs, not just DefaultPlanExecutor. While the early-return guard limits the overhead, registering a listener on all types is a performance concern in large Guice injectors. Consider using a more specific matcher (e.g., Matchers.subclassesOf(DefaultPlanExecutor.class)) to limit the scope.

b.bindListener(Matchers.any(), new TypeListener() {
    @Override
    public <I> void hear(TypeLiteral<I> type, TypeEncounter<I> encounter) {
        if (!DefaultPlanExecutor.class.isAssignableFrom(type.getRawType())) {
            return;
        }
        encounter.register((InjectionListener<I>) instance -> pushAnalyticsServices((DefaultPlanExecutor) instance));
    }
});
Missing SPI File

The Javadoc mentions that implementers are declared via standard Java SPI (META-INF/services), but no META-INF/services/org.opensearch.analytics.spi.AnalyticsFrontEndExtension file is included in the PR. Without this file, the SPI discovery mechanism will not work unless the ExtensiblePlugin loader handles it differently. This should be verified or the file should be added.

public interface AnalyticsFrontEndExtension {

    /**
     * Receives the bundle of analytics-engine services. Called exactly once after the services are
     * constructed and before any analytics query is dispatched. Each service inside the bundle is
     * safe to invoke from any thread once received.
     */
    void setAnalyticsServices(AnalyticsServices services);

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Narrow type listener scope to avoid unnecessary overhead

Using Matchers.any() causes the TypeListener to be invoked for every type Guice
instantiates, which is unnecessarily broad and adds overhead. Use
Matchers.subclassesOf(DefaultPlanExecutor.class) to scope the listener only to the
relevant type, avoiding the per-type isAssignableFrom check and reducing noise.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/AnalyticsPlugin.java [129-137]

-b.bindListener(Matchers.any(), new TypeListener() {
+b.bindListener(Matchers.subclassesOf(DefaultPlanExecutor.class), new TypeListener() {
     @Override
     public <I> void hear(TypeLiteral<I> type, TypeEncounter<I> encounter) {
-        if (!DefaultPlanExecutor.class.isAssignableFrom(type.getRawType())) {
-            return;
-        }
         encounter.register((InjectionListener<I>) instance -> pushAnalyticsServices((DefaultPlanExecutor) instance));
     }
 });
Suggestion importance[1-10]: 6

__

Why: Using Matchers.any() invokes the TypeListener for every Guice-managed type, adding unnecessary overhead. Scoping it with Matchers.subclassesOf(DefaultPlanExecutor.class) is more precise and eliminates the redundant isAssignableFrom check, improving performance and clarity.

Low
Possible issue
Use atomic flag for thread-safe one-time initialization

The servicesPushed flag is a plain boolean field, but pushAnalyticsServices is
synchronized. However, the TypeListener/InjectionListener callback may be invoked
from a different thread than the one that reads servicesPushed outside the
synchronized block. Using AtomicBoolean with compareAndSet would be safer and more
idiomatic for this guard pattern, eliminating any risk of a data race on the flag
itself.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/AnalyticsPlugin.java [141-147]

-private boolean servicesPushed = false;
+private final AtomicBoolean servicesPushed = new AtomicBoolean(false);
 
-private synchronized void pushAnalyticsServices(DefaultPlanExecutor executor) {
-    if (servicesPushed) {
+private void pushAnalyticsServices(DefaultPlanExecutor executor) {
+    if (!servicesPushed.compareAndSet(false, true)) {
         return;
     }
-    servicesPushed = true;
+    SchemaProvider schemaProvider = OpenSearchSchemaBuilder::buildSchema;
+    AnalyticsServices services = new AnalyticsServices(executor, schemaProvider);
+    for (AnalyticsFrontEndExtension consumer : frontEnds) {
+        try {
+            consumer.setAnalyticsServices(services);
+        } catch (Exception e) {
+            logger.warn("AnalyticsFrontEndExtension {} threw on setAnalyticsServices", consumer.getClass().getName(), e);
+        }
+    }
+}
Suggestion importance[1-10]: 5

__

Why: The synchronized method already provides thread safety for servicesPushed, so the AtomicBoolean suggestion is a style/idiom improvement rather than a critical fix. The current synchronized approach is functionally correct, making this a minor improvement.

Low
Remove potentially unsafe redundant cast in lambda

The lambda casts clusterState to ClusterState unnecessarily since the SchemaProvider
interface's parameter is already typed. If SchemaProvider accepts a raw Object or a
different type, this cast could throw a ClassCastException at runtime. Verify the
SchemaProvider functional interface signature and remove the redundant cast, or use
a proper method reference instead.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/AnalyticsPlugin.java [148]

-SchemaProvider schemaProvider = clusterState -> OpenSearchSchemaBuilder.buildSchema((ClusterState) clusterState);
+SchemaProvider schemaProvider = OpenSearchSchemaBuilder::buildSchema;
Suggestion importance[1-10]: 4

__

Why: The cast (ClusterState) clusterState may be redundant or unsafe depending on the SchemaProvider interface signature. However, without seeing the SchemaProvider interface definition, it's unclear if a method reference would compile directly. The suggestion is valid but may require verification of the interface signature.

Low

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for d919b94: SUCCESS

@codecov

codecov Bot commented May 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.48%. Comparing base (e089d06) to head (d919b94).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21453      +/-   ##
============================================
+ Coverage     73.38%   73.48%   +0.10%     
- Complexity    74353    74429      +76     
============================================
  Files          5966     5966              
  Lines        338131   338131              
  Branches      48740    48740              
============================================
+ Hits         248139   248479     +340     
+ Misses        70247    69813     -434     
- Partials      19745    19839      +94     

☔ 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.

@ahkcs ahkcs closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant