Skip to content

Add AnalyticsFrontEndExtension SPI + AnalyticsServices bundle for analytics-engine frontend integration - #21449

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

Add AnalyticsFrontEndExtension SPI + AnalyticsServices bundle for analytics-engine frontend integration#21449
ahkcs wants to merge 1 commit into
opensearch-project:mainfrom
ahkcs:feature/analytics-extension-spi

Conversation

@ahkcs

@ahkcs ahkcs commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an SPI interface and a services bundle in analytics-framework that let frontend plugins (e.g., opensearch-sql) integrate with analytics-engine without taking a hard install-time dependency on it.

Mirrors the JobSchedulerExtension pattern from opensearch-job-scheduler: the frontend plugin declares its capability via AnalyticsFrontEndExtension; the publishing plugin (analytics-engine) discovers consumers via ExtensiblePlugin#loadExtensions and pushes an AnalyticsServices bundle to each consumer once Guice has constructed them.

Why

Today, opensearch-sql declares extendedPlugins = [..., 'analytics-engine'] as a HARD dependency. This causes the SQL plugin install to fail on stock OpenSearch distros that don't ship analytics-engine (Missing plugin [analytics-engine], dependency of [opensearch-sql]). 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

Two new files in sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/:

  • AnalyticsFrontEndExtension — the SPI interface. Single method:
    void setAnalyticsServices(AnalyticsServices services);
  • AnalyticsServices — the services bundle (record). Initial fields:
    • QueryPlanExecutor<RelNode, Iterable<Object[]>> queryPlanExecutor
    • SchemaProvider schemaProvider (already exists in analytics-framework)

Bundled rather than separate setters so future analytics-engine services can be added without changing the SPI signature — frontends that don't consume the new service simply ignore the new accessor.

JavaDoc on the interface spells out the lifecycle (discovery via ExtensiblePlugin#loadExtensions, push after Guice builds the node injector, exactly once per consumer per node, before first analytics query).

What's NOT in this PR

  • No producer-side wiring in AnalyticsPlugin (loadExtensions collection + Guice listener for DefaultPlanExecutor + push to consumers). That's a follow-up by the analytics-engine team.
  • No frontend consumer changes — those land in opensearch-sql separately.

Related

Coordination thread: opensearch-project/sql#5398 — the SQL-side draft of this same interface, which gets deleted once this PR merges and a new analytics-framework JAR is vendored into opensearch-sql.

Test plan

  • ./gradlew :sandbox:libs:analytics-framework:compileJava -Dsandbox.enabled=true — passes.

The interface has no implementations or callers in this PR, so there are no behavioral tests.

@ahkcs
ahkcs requested a review from a team as a code owner May 1, 2026 01:27
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 07ecf84)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Lifecycle Guarantee

The JavaDoc states setAnalyticsServices is called "exactly once per consumer per node lifecycle" and "BEFORE the first analytics query is dispatched." However, there is no enforcement mechanism in the interface itself (e.g., no default guard, no contract validation). If the publishing plugin (analytics-engine) calls this method more than once, or if a consumer invokes analytics services before the callback arrives, there is no protection. Consider documenting or enforcing idempotency/immutability expectations, or providing a default method that throws on double-invocation.

 * <p><b>Lifecycle.</b> {@link #setAnalyticsServices} is invoked exactly once per consumer per node
 * lifecycle, AFTER the node Guice injector is built (i.e., after every plugin's
 * {@code createComponents} returns) and BEFORE the first analytics query is dispatched.
 * Implementations should stash the bundle for later use; do not assume the services are available
 * during {@code createComponents}.
 *
 * @opensearch.internal
 */
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);
Null Safety

The AnalyticsServices record has no null-checks on its fields. If analytics-engine is partially initialized or a service fails to construct, a partially-populated bundle could be pushed to consumers, leading to NullPointerExceptions at query time. Consider adding compact constructor validation (e.g., Objects.requireNonNull) for both queryPlanExecutor and schemaProvider.

public record AnalyticsServices(QueryPlanExecutor<RelNode, Iterable<Object[]>> queryPlanExecutor, SchemaProvider schemaProvider) {
}
Leaking Internal Types

The record exposes QueryPlanExecutor<RelNode, Iterable<Object[]>> with a concrete Calcite type (RelNode) in the SPI's public API surface. This creates a transitive compile-time dependency on calcite-core for every frontend plugin that imports AnalyticsServices, even if the frontend never directly uses RelNode. Consider whether an abstraction or opaque handle would be more appropriate for an SPI boundary.

public record AnalyticsServices(QueryPlanExecutor<RelNode, Iterable<Object[]>> queryPlanExecutor, SchemaProvider schemaProvider) {
}

@ahkcs
ahkcs force-pushed the feature/analytics-extension-spi branch from 0e1c142 to bda1e46 Compare May 1, 2026 01:33
@ahkcs ahkcs changed the title Add AnalyticsExtension SPI for analytics-engine frontend integration Add AnalyticsFrontEndExtension SPI for analytics-engine frontend integration May 1, 2026
@ahkcs
ahkcs force-pushed the feature/analytics-extension-spi branch from bda1e46 to 91a6d1f Compare May 1, 2026 01:35
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit bda1e46

@ahkcs ahkcs changed the title Add AnalyticsFrontEndExtension SPI for analytics-engine frontend integration Add AnalyticsFrontEndExtension SPI + AnalyticsServices bundle for analytics-engine frontend integration May 1, 2026
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 91a6d1f

Adds an SPI interface 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.

Mirrors the JobSchedulerExtension pattern from opensearch-job-scheduler:
the frontend plugin declares its capability via AnalyticsFrontEndExtension;
the publishing plugin (analytics-engine) discovers consumers via
ExtensiblePlugin#loadExtensions and pushes an AnalyticsServices bundle
to each consumer once Guice has constructed them.

Bundled services (rather than separate setters) so analytics-engine can
add new services in the future without changing the SPI signature.
Initial bundle:
  - QueryPlanExecutor<RelNode, Iterable<Object[]>>
  - SchemaProvider

Lifecycle (documented on the interface): setAnalyticsServices is called
exactly once per consumer per node, after every plugin's createComponents
returns and before the first analytics query is dispatched.

This PR adds only the contract. The producer-side wiring in
AnalyticsPlugin (loadExtensions + Guice listener + push) and the
opensearch-sql consumer-side implementation will follow.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
ahkcs force-pushed the feature/analytics-extension-spi branch from 91a6d1f to 07ecf84 Compare May 1, 2026 01:59
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 07ecf84

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null validation for record components

The record does not validate that its components are non-null. If either
queryPlanExecutor or schemaProvider is null when passed to setAnalyticsServices,
consumers will receive a partially-initialized bundle and encounter
NullPointerExceptions at query time. Add a compact canonical constructor that
performs null checks.

sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/AnalyticsServices.java [28-29]

 public record AnalyticsServices(QueryPlanExecutor<RelNode, Iterable<Object[]>> queryPlanExecutor, SchemaProvider schemaProvider) {
+    public AnalyticsServices {
+        Objects.requireNonNull(queryPlanExecutor, "queryPlanExecutor must not be null");
+        Objects.requireNonNull(schemaProvider, "schemaProvider must not be null");
+    }
 }
Suggestion importance[1-10]: 6

__

Why: Adding null checks in a compact canonical constructor is a valid defensive programming practice that prevents NullPointerExceptions at query time. However, this is a minor improvement and the impact is limited since the calling code in AnalyticsPlugin should already ensure non-null services.

Low
General
Enforce single-invocation contract at interface level

The method contract states it is "called exactly once," but the interface provides
no enforcement mechanism. A default implementation that throws IllegalStateException
on a second invocation, or at minimum a null-check guard, would protect consumers
from accidental double-initialization bugs introduced by future callers.

sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/AnalyticsFrontEndExtension.java [41]

+/**
+ * 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.
+ *
+ * @param services the analytics services bundle; must not be {@code null}
+ * @throws IllegalArgumentException if {@code services} is {@code null}
+ * @throws IllegalStateException    if called more than once
+ */
 void setAnalyticsServices(AnalyticsServices services);
Suggestion importance[1-10]: 1

__

Why: The improved_code only adds Javadoc comments to the method signature without actually implementing any enforcement mechanism (like a default method with state tracking). The suggestion claims to enforce the single-invocation contract but the improved code doesn't add any runtime enforcement, making it essentially a documentation-only change.

Low

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 07ecf84: 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?

ahkcs added a commit to ahkcs/sql that referenced this pull request May 1, 2026
Per Peter's review on opensearch-project#5400: flag the three new httpcore5/httpclient5
exclusions (and ideally the entire bundlePlugin exclusion block) for
removal once analytics-engine becomes an optional dependency via the
AnalyticsFrontEndExtension SPI in opensearch-project/OpenSearch#21449.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs closed this May 1, 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