-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[RFC] Cloud Native SQL Plugin #13274
Comments
I like this proposal because it's effectively identical to https://github.com/opensearch-project/opensearch-sdk-java except that you are offering to refactor core vs. exposing a new interface on top of core. Generally, refactoring core is a costly undertaking, everything takes 10x more work. We're seeing that even relatively small changes to enable things like #10684 require weeks of iterating, just to get gradle checks to pass. You also will need to deal with things like settings, therefore the proposed plugin interface should include that (and possibly other things). Wouldn't it be easier to make https://github.com/opensearch-project/opensearch-sdk-java the plugin interface you're proposing and implementing the SQL plugin on top of that? If you shortcut/remove/bypass all the remote aspects of that SDK, you effectively have a clean plugin interface, and you can iterate on that completely independently from core. |
@dblock's comments about SDK prompted me to think of the fact that we really wanted to implement those using OpenSearch Java Client. That continues on to the whole conversation around generated code and reference specifications. Which brings me to the question: do we need to write these interfaces or can we generate them from the OpenAPI spec? |
I think we can. It's a ton of boilerplate. That project has moved ahead quite well! https://github.com/opensearch-project/opensearch-api-specification @Xtansia is working on a java generator from API, it could generate anything |
Hey folks! Thanks for the comments! To summarize my understanding: @dblock's feedback is that we should use the opensearch-sdk-java as our target for a cloud native sql plugin.
@dbwiddis' feedback is that we should generate the core api interface using the API spec. I've taken a look at the opensearch-sdk-java and I do believe this would offer us a path
In a similar vein, a solution which I have considered is migrating the SQL plugin to the I see this as a similar approach to the opensearch-sdk-java as both solutions are API based and Given that this Client is already in core and exposed to plugins, I would not expect additional @dblock @saratvemulapalli I want to understand another point: I see in the opensearch-sdk-java that the To add to the API discussion. I've been having discussions with @shwetathareja on decomposing cluster To be compatible with her RFC here: #13197 we would
This is fine grained access is particularly a problem for cluster state because
These APIs changes would be applicable to both SDK and Client based approaches. Looking forward to hearing your thoughts! |
Thanks for keeping an open mind @fddattal! In picking similar-ish solutions in terms of complexity, risk and cost I would always take the one that's viable most long term.
Correct. There's probably a very minimal solution that assumes that both sides are the same version of the message being sent and passes it in-memory rather than through serialization/deserialization.
I think you are looking for opensearch-project/opensearch-sdk-java#732 that's all @dbwiddis. |
Hey folks! I've taken a look at #13336 and I think that there's a shared solution that would position us well in the long term for a full extension migration. ProposalWhat I am proposing is the following:
Tenets
Interface Definitionpackage org.opensearch.sdk;
import org.opensearch.sdk.model.ClearScrollRequest;
import org.opensearch.sdk.model.ClearScrollResponse;
import org.opensearch.sdk.model.DeleteCustomRequest;
import org.opensearch.sdk.model.DeleteCustomResponse;
import org.opensearch.sdk.model.GetCustomRequest;
import org.opensearch.sdk.model.GetCustomResponse;
import org.opensearch.sdk.model.GetIndexMappingsRequest;
import org.opensearch.sdk.model.GetIndexMappingsResponse;
import org.opensearch.sdk.model.GetIndexSettingRequest;
import org.opensearch.sdk.model.GetIndexSettingResponse;
import org.opensearch.sdk.model.GetSystemSettingRequest;
import org.opensearch.sdk.model.GetSystemSettingResponse;
import org.opensearch.sdk.model.MultiSearchRequest;
import org.opensearch.sdk.model.MultiSearchResponse;
import org.opensearch.sdk.model.PutCustomRequest;
import org.opensearch.sdk.model.PutCustomResponse;
import org.opensearch.sdk.model.ResolveIndicesAndAliasesRequest;
import org.opensearch.sdk.model.ResolveIndicesAndAliasesResponse;
import org.opensearch.sdk.model.ScrollRequest;
import org.opensearch.sdk.model.SearchRequest;
import org.opensearch.sdk.model.SearchResponse;
import java.util.concurrent.CompletionStage;
public interface Client {
// for sql plugin
CompletionStage<SearchResponse> search(SearchRequest request);
CompletionStage<SearchResponse> scroll(ScrollRequest request);
CompletionStage<ClearScrollResponse> clearScroll(ClearScrollRequest request);
CompletionStage<GetIndexMappingsResponse> getIndexMappings(GetIndexMappingsRequest request);
CompletionStage<GetIndexSettingResponse> getIndexSetting(GetIndexSettingRequest request);
CompletionStage<GetSystemSettingResponse> getSystemSetting(GetSystemSettingRequest request);
CompletionStage<ResolveIndicesAndAliasesResponse> resolveIndicesAndAliases(ResolveIndicesAndAliasesRequest request);
CompletionStage<MultiSearchResponse> multiSearch(MultiSearchRequest request);
// for data store interface
CompletionStage<PutCustomResponse> putCustom(PutCustomRequest request);
CompletionStage<GetCustomResponse> getCustom(GetCustomRequest request);
CompletionStage<DeleteCustomResponse> deleteCustom(DeleteCustomRequest request);
} Pros / ConsPros:
Cons:
What's Not Addressed Here
|
@fddattal I like this. Do you think it makes sense to reuse https://github.com/opensearch-project/opensearch-sdk-java for that and possibly produce multiple artifacts (that eventually merge)? |
@dblock I like that approach and it's certainly feasible to do so. We would modify the SDK package to be a multi-module gradle project, and the existing extension code would become one module, while the API would be another. Each releasing their own artifacts. |
I've put together a POC for this approach here:
I was able to:
I found I spent a lot of time creating the model objects and writing the code to convert to/from the model and core data structures. I think having some standard tooling in the API to aide with this would help drive adoption. |
@fddattal : one clarification, these Pojo are re-using existing OpenSearch cluster state sub objects/ data structures or altogether new Pojo's |
@shwetathareja These Pojos are new altogether and would live outside of core in the SDK package. |
Next StepsHey folks, I don't see any blocking concerns so I am moving ahead with a full implementation. Below is what to expect regarding high level changes. Please let me know if you have any concerns! Investigation Work
OpenSearch Java SDK
SQL Plugin
Core
|
@fddattal can you elaborate why :server module would need this dependency?
How are you going to generate these Pojo? Using OpenAPI specification? What would be the performance overhead of translating across objects? |
Thanks for your comments @shwetathareja !
Core :server needs to provide an implementation of the
For now, I plan to hand write the Java classes by hand and use Lombok to generate the boilerplate. We certainly could use a spec to generate the API in the future as more target runtimes are supported.
I've benchmarked (on my laptop) translating a large search request with 10k boolean clauses and a large search response with 10k hits and found the request took 0.03 ms and response took 0.1 ms. If performance overhead due to object translation becomes a problem we could introduce interfaces in the API which map 1:1 with their corresponding core objects and have the core object implement those interfaces. For such implementations, the translation would be a noop.
|
Sorry to chime late. @fddattal since the RFC is around running SQL independently than core. And one of option provided was opensearch-sdk-java. 2 years back when we started working on sdk, SQL was the first plugin I integrated with sdk to run independently. Commit history of my fork for any help around. |
@owaiskazi19 whoa I totally forgot about that one! full circle |
@fddattal I'm assuming "core" here means the |
Thanks for your comment @andrross !
Yes it's true that "core" means the I'd state our goal slightly differently - Refactor the read-only paths in SQL plugin which access core to one unifying cloud native interface. In doing so we would remove deep core access channels such as direct cluster state access and unnecessary core details being exposed to the plugin. The steps taken here will help us to fully remove the core dependency in the future. However, the work will not be sufficient to do so because both non-read-only paths and accesses from core to plugin will not be refactored. |
@fddattal We currently have the |
Thanks @fddattal for the RFC. Here are my thoughts and proposal: Tenets (in addition to the ones listed by @fddattal)
I'd like this interface to de-couple plugins in the following verticals, and deliver in phases:
I would want to deliver these verticals in phases and incrementally. Eventually once we have adoption we can explore the options of deprecating existing access's to the core I would like to have the interface defined in OpenSearch repo as a library and default implementation in Few things I'd like to push it down the road:
I would also like to get feedback from @reta @AmiStrn @samuel-oci @rursprung @abseth-amzn who worked in the area of plugins. [1] #2447 |
I think that's it. We put it in a different repo (a facade) to reduce blast radius when API/implementation/internals change in core. Instead of having to figure out how to fix N plugins we fix the SDK, which itself then depends on a stable version of core rather than on the next moving version of core. |
@saratvemulapalli I think we're aligned and I really like how you've broken much of this work down. A couple points to follow up on though:
I agree with this. Probably something like a new library in
Just to clarify, as we build on the vision and work started in #5910 we should be moving many parts of the code in |
@dbwiddis That's a good catch! I think its in our best interest to define two separate APIs for modifying index level and cluster level customs. What are your thoughts? |
@fddattal I've already started down the road of implementing against the proposed I can work with either and am focusing on the lower-level implementations first, but I'd really like us to align on an interface style sooner rather than later. I thought we had. The The interface is also completely independent of core, as it uses base Java functionality Your new proposal requires changing method arguments for the interface if an additional field is required. I don't like this at all. I strongly prefer the request/response object model. I'm flexible on including ActionListeners and there's some benefit to Can we align on an approach? |
Sorry to chime in late, I agree with @reta we shouldn't be doing pluginsV2, there are already too many things in place. In particular I would like to understand better what kind of specific problem this suggestion is solving. |
Absolutely @samuel-oci! The particular problem that I have comes down to the fact that I deploy a custom distribution of OpenSearch. In my distribution, cluster based components don't exist. So, all code which depends on those direct cluster object do not work. For example the following would not work because accessing MappingMetadata mappings = clusterService.state().metadata().index("foo").mapping(); So what I am proposing is we introduce an interface to abstract these core details to allow plugins to run Taking the same example, you can imagine an interface that exposes an operation: CompletionStage<GetMappingMetadataResponse> getMappingMetadata(GetMappingMetadataRequest);
// request/response objects
class GetMappingMetadataRequest {
String index;
}
class GetMappingMetadataResponse {
MappingMetadata metadata;
} In the standard OpenSearch distribution, this interface would just thinly wrap the existing access pattern via cluster service. However, if I had this interface, then I would cleanly be able to stub out its implementation so that it works in my custom distribution and the plugin would then be able to run without a fork. The full extend of what this interface would need to support for the SQL plugin is detailed in this comment: #13274 (comment) OpenSearch core would likewise benefit in having this interface because of the greater degree of decoupling which it provides.
Agreed @dbwiddis ! I am also inclined to prefer CompletionStage's and the request/response object model! I am happy to standardize on this style. |
@reta @samuel-oci The "plugin V2" term isn't really precisely defined and can have different meaning to different folks so I'm going to avoid using it (I definitely think reimplementing the whole plugin interface would be the wrong approach anyway...) What @fddattal is describing here seems pretty reasonable to me. He wants to run the SQL plugin in some way where the index metadata comes from some place other than cluster state, so tightly coupling the SQL plugin code to call The primary question as I see it is where should this interface live? It absolutely could be a library completely external to OpenSearch core that plugins could choose to use if they want and it could evolve independently from core. However, the SQL plugin likely isn't the only plugin that could benefit from reduced compile-time dependency on core details. I see this as a potential opportunity to add something like an "IndexMetadataProvider" interface in the plugin API that could replace plugins' tight coupling on the entire ClusterService public Java API, for example. So should this be a part of an effort by the core to present a more constrained API to plugins? I'll also note that the general shape of the problem is the same that @dbwiddis is looking at, except in his case he's dealing with wanting a more abstract way to store plugin metadata that isn't tightly coupled to the concept of a system index or cluster state. |
@andrross @fddattal Not sure I understand the "why", in what scenario this is helpful to not be able to access cluster state from a plugin? If you want to run a plugin not from within an OpenSearch node why not just leverage extensions? |
@samuel-oci why would we have to rewrite a core plugin to a new extensions interface when we just want to abstract some state namely cluster state? it seems like an overkill, am i missing something |
Thanks @anastead , I think we have 2 alternative routes to pursue, largely with following tradeoffs:
@fddattal @dbwiddis (and I think you as well) lean towards the 2nd option: do less (if anything) on SQL plugin, do more on OpenSearch core side. To be fair, I am OK with the approach, but I am not satisfied with the APIs being suggested to become part of OpenSearch. The API is very much oriented at specific narrow problem at hand, or to say it in another words - bringing somethings you need right now and who knows what is coming next. The functionality of |
@reta I do not understand why we would like to prevent access to cluster state from a plugin? unless we are not planning to run it from within OpenSearch node. (I think I am missing something? :) ). I am just trying to understand the benefits of it and understand the "why" part.
This is my feeling as well, but I try not to jump straight away into that conclusion, which is why I am trying to make sure I understand the use case. |
Thanks @samuel-oci
We are not preventing that, I think the "cluster state" is somewhat misleading here, the plugin needs cluster wide data (source of truth) which is already exposed / could be accessed through different means. |
This sounds like a very specific problem in a private modified opensearch code running somewhere in @fddattal environment. What's the incentive to change public core in that case? |
Great question! As @fddattal stated "This proposal aids in decoupling plugins from OpenSearch Core by restricting their core access through an abstract interface...". But I agree with @reta that the details matter here. If the proposed interface doesn't get us towards those goals then it probably should not go in core. For me, providing a way for plugins to get index metadata (a common use case) that doesn't require interacting with the entirety of ClusterService and ClusterState seems like a win for decoupling. |
I re-read the previous comments and I think I better understand the intention now. So let me try again :) |
I want to add my 2 cents to the discussion: Today, OpenSearch Cluster Manager has multiple responsibilities like metadata management, shard management, health checks, node membership, serving admin APIs along with the overall cluster management. One of the goal which RFC #13197 aims to achieve is to refactor the ClusterManager in a way to separate out the metadata management (along with pluggable storage) from other aspects of the cluster as the first step. It could enable cloud native providers to simply build on top metadata management lib and may not need to set it up as a cluster infrastructure. Lets take create-index API as an example: Now, the intention is to separate out the validations, applying the template and generate the new index object (resource) as Metadata management, processing this resource to generate the new over all cluster state as cluster management aspect and finally the pluggable storage layer (local or remote). The Shard management can itself be separated out. But, for now lets keep it out of the discussion. With this context, Plugins can have both read and write access to Cluster State. What @fddattal wants to separate out the read access to Cluster State or simply put the metadata access layer as the first step. So, this metadata access layer has to be inside core. Also, I don't think we should be doing un-necessary transformation by creating new Pojos. The access layer performance characteristics with in the cluster setup should be almost as same as access to ClusterService.state() object. |
Thanks @shwetathareja, we basically need to make Cluster Management core APIs fine grained (fe cluster view vs cluster management), right? I think this is would be useful and than we could may be introduce more lightweight
It has |
Thanks @reta yes, you captured the essence right 👍 more on the lines of Metadata. @fddattal was exploring the performance characteristics of NodeClient. Yes, we can use executeLocally so the transport overhead goes away but lets take |
Got it, thanks. So I think we should take a step back and focus on Cluster Management APIs first, that would help us to design the plugin related abstractions after. |
I've created a draft PR containing the code for this proposal. Please take a look! #13712 |
We are currently trying to finalize the new feature process so the process is not fully in place yet. But a few notes on this discussion so far:
|
Hey folks! I want to let you all know that I've decided to pursue a path which does not require modifications to OpenSearch core. I will be vending a custom Thanks you for all of your inputs! It's great to see such a lively discussion! |
Is your feature request related to a problem? Please describe
I want to run the SQL plugin in a cloud native environment, but I am unable to do so without forking the code.
Today, plugin execution is tightly coupled to OpenSearch core. Access to metadata of index, aliases, templates, node roles, and routing is tightly coupled with cluster state. Furthermore, plugins are vended full access to OpenSearch core including direct access to cluster state internals. This make it difficult to evolve plugin APIs to be more cloud native provider friendly, and eventually run plugins in a stateless mode.
Describe the solution you'd like
I would like to propose an interface to plugins for accessing OpenSearch core. The interface should expose the minimal fine grained state management operations and the operations needed to support plugins. This will ensure both cluster and cloud native implementation can support the plugin uniformly.
Then the SQL plugin will migrate to this interface and be enabled to run in cloud native environments.
Related component
Plugins
Describe alternatives you've considered
No response
Additional context
FAQ
How would the community benefit from this?
2.a. The possibility for alternative implementations of core APIs.
2.b. Improve stability guarantees of the plugin APIs.
2.c. Complete audibility of plugin’s access to core APIs via this interface.
2.d. The enabling of alternative runtime environments to run plugins unmodified.
What other RFCs does this align with?
1.a. This work adds upon the RFC proposed for cluster state management. Similar to how cluster state management is being refactored to enable cloud native environments, so will plugin to core access.
2.a. In addition to code separation, a standard for plugin to core interaction is now defined to make core module implementations swappable.
3.a. In a future where all plugin to core access is done via the interface. Security negotiation can be mediated by the interface and eventually lead to the deprecation of the mutable thread context for security management.
The text was updated successfully, but these errors were encountered: