Skip to content

spike refactor of logstashbridge stable API #1

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

Conversation

yaauie
Copy link

@yaauie yaauie commented Jul 21, 2025

This is a spike that does a few things on top of your adoption spike in response to the LS-filter PR. I can go back and make smaller commits if it helps.

Internal & External vs Unwrap & Wrap

It is easy to get lost as to what wrap and unwrap mean, especially since we are actually implementing some of these bridge interfaces over in the LS-filer.

Uses "internal" and "external" terminology throughout.

  • the type parameter of StableBridgeAPI<T> becomes StableBridgeAPI<INTERNAL> to make it clear that the type we mirroring is the elasticsearch-internal type
  • methods that consume an elasticsearch-internal type to produce a bridged type change from wrap to fromInternal
  • methods that consume a bridged type to produce an elasticsearch-internal type change from unwrap to toInternal
  • the StableBridgeAPI.Proxy<T> class—which implements StableBridgeAPI<T> becomes StableBridgeAPI.ProxyInternal<INTERNAL> to make it clearer that we are proxying an elasticsearch-internal object

To accommodate this change in your your stable bridge adoption spike:

  • /s/unwrap/toInternal/g
  • /s/wrap/fromInternal/g

Abstract Base Class of ProcessorBridge for external implementations

Adds an abstract base class for external implementations called ProcessorBridge.AbstractExternal

  • handles the toInternal by providing an ProxyExternal that satisfies the elasticsearch-internal Processor by proxying through to the external implementation of the PipelineBridge
  • ensures that we don't nest proxies-of-proxies -- the ProcessorBridge.fromInternal(Processor) intercepts these external-proxies and returns their wrapped external bridge implementation directly instead of double-wrapping them

To accommodate this: PipelineProcessor will need to extend ProcessorBridge.AbstractExternal instead of implementing ProcessorBridge.

Better commentary for classes and interfaces.

Resolved a lot of code-style warnings by describing the function of classes

 - transitions terminology from wrap/unwrap to toInternal/fromInternal
 - adds abstract base class for ProcessorBridge, since we are expecting external
   implementations, which includes an internal-shaped proxy to the external
   definition.
 - adds copious commentary for the classes that were previously shipped
Copy link
Owner

@mashhurs mashhurs left a comment

Choose a reason for hiding this comment

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

I don't see big change here. The conceptual modification makes sense to me that any external class usages go through the proxy interface to the bridge and when it needs it can point to internal (ES classes) without importing them.

* the details of maintaining a singular internal-form implementation of {@link Processor}
* that proxies calls through the external implementation.
*/
abstract class AbstractExternal implements ProcessorBridge {
Copy link
Owner

Choose a reason for hiding this comment

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

It looks any processors who is going to align on ProcessorBridge needs to extend this abstract class. Sounds reasonable!

@mashhurs
Copy link
Owner

FYI: I have compiled the bridge (./gradlew :libs:logstash-bridge:build) and got failure on style checks, I will fix it once we agree on going together with upstream PR.

@mashhurs mashhurs merged commit ba80d06 into mashhurs:move-to-bridge-stable-api-investigation Jul 22, 2025
4 checks passed
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.

2 participants