opensearch-project / opensearch-plugins

For all things OpenSearch plugins. You want to install, or develop a plugin? You've come to the right place.
Apache License 2.0
51 stars 61 forks source link

Migrating plugin API endpoints #13

Closed ohltyler closed 3 years ago

ohltyler commented 3 years ago

Currently, all plugins have followed the same convention of prefixing all APIs with _opendistro/* to represent that they were APIs supported by the Open Distro for Elasticsearch project. Now, these plugins are more closely associated with the core OpenSearch/OpenSearch Dashboards source code itself.

One option is removing the _opendistro prefix entirely, and allowing the OpenSearch plugins to shorten and simplify their paths. Example: _opendistro/_anomaly_detection/* -> _anomaly_detection/*.

Another option is to replace _opendistro with _opensearch, but I feel this may be a bit unnecessary and repetitive in this case.

If something like the first option is decided upon, we may apply similar strategies for other areas that need migration from opendistro, including system indices, cluster settings, etc. as mentioned in #12

Creating this issue as a point of discussion on possible solutions to migrate the current plugin REST APIs.

saratvemulapalli commented 3 years ago

Thanks for the issue @ohltyler! Great thought, love the idea.

My thoughts on this:

  1. All plugins part of the opensearch-project are first class citizens and having the brand of opensearch in APIs, indices, namespaces etc helps us differentiate.
  2. The customers who were already running ODFE plugins, can intuitively rename them to opensearch.

I would prefer to stick with _opensearch and as you said this will have similar implications in other places like indices, ENV variables etc.

Here is a similar discussion in security plugin: https://github.com/opensearch-project/security/pull/1149#issuecomment-832409159

ylwu-amzn commented 3 years ago

By adding _opensearch, we can provide specific namespace for all opensearch plugins. It may provide some convenience for future extension to support custom plugins from third party. For example, community build similar anomaly detection plugin and want to install it into OpenSearch cluster together with our OpenSearch AD plugin. We can avoid name conflict for API, index and cluster settings by adding _opensearch.

adnapibar commented 3 years ago

By adding _opensearch, we can provide specific namespace for all opensearch plugins. It may provide some convenience for future extension to support custom plugins from third party. For example, community build similar anomaly detection plugin and want to install it into OpenSearch cluster together with our OpenSearch AD plugin. We can avoid name conflict for API, index and cluster settings by adding _opensearch.

While I agree, on the other hand it also makes it harder for clients using this API to migrate in the future, doesn't it? What If Elasticsearch had _elasticsearch prefix in api signatures?

ohltyler commented 3 years ago

Seems that the different opinions rely on how closely we want to tie these plugins to OpenSearch the software vs. OpenSearch the project/organization.

Seems to me that:

ylwu-amzn commented 3 years ago

I think either solution can extend in some way to support future use cases. Just a matter of effort and who will pay for the effort. Need to consider the long term goal and vision to minimize the future effort and make tradeoff. Community user may have different opinions. Let's collection more feedback from community.

stockholmux commented 3 years ago

Food for thought:

Does the namespacing as _opensearch or no namespacing at all put these plugins at an advantage that no one else gets?

The 'first-party' plugins probably should play by the same rules as anyone else. E.g. If another organization creates a competing functionality, does the namespacing proposed create an advantage? If someone else created a plugin without namespacing would you be okay with this?

This might enter the danger zone on principles 3 and 8.

Regardless, I would make sure and post this in the forums for a wider reach - I'm not sure how many folks are hanging out in this repo.

cliu123 commented 3 years ago

Thanks for the issue @ohltyler! Great thought, love the idea.

My thoughts on this:

  1. All plugins part of the opensearch-project are first class citizens and having the brand of opensearch in APIs, indices, namespaces etc helps us differentiate.
  2. The customers who were already running ODFE plugins, can intuitively rename them to opensearch.

I would prefer to stick with _opensearch and as you said this will have similar implications in other places like indices, ENV variables etc.

Here is a similar discussion in security plugin: opensearch-project/security#1149 (comment)

@saratvemulapalli While this is a good topic to discuss on API paths, but the discussion in security plugin is not about API paths. It is about the class name, method name and variable name. The requirement in the SOP for migration is to rename all opendistro prefixes to "opensearch". Is that a requirement consistently apply to all plugins? Or the prefixes can be removed? Personally, I would like all plugins to follow the same naming conventions even for class name, method name and variable name. Can we have a consistent requirements on this for all plugins as soon as we can? We have a pending PR for those changes waiting for a decision.

dblock commented 3 years ago

@cliu123 Do you need a section for class names, method names and variable names in https://github.com/opensearch-project/opensearch-plugins/blob/main/UPGRADING.md?

cliu123 commented 3 years ago

@cliu123 Do you need a section for class names, method names and variable names in https://github.com/opensearch-project/opensearch-plugins/blob/main/UPGRADING.md?

@dblock Yes, that would help. IMO, it would be good to have consistent naming conventions cross all plugins. I changed all “opendistro” prefixes to “OpenSearch”, but there’s a discussion on whether the prefixes should be removed on the PR(https://github.com/opensearch-project/security/pull/1149#issuecomment-831395827).

saratvemulapalli commented 3 years ago

@cliu123 Do you need a section for class names, method names and variable names in https://github.com/opensearch-project/opensearch-plugins/blob/main/UPGRADING.md?

@dblock Yes, that would help. IMO, it would be good to have consistent naming conventions cross all plugins. I changed all “opendistro” prefixes to “OpenSearch”, but there’s a discussion on whether the prefixes should be removed on the PR(opensearch-project/security#1149 (comment)).

Sure, we can cover this in the issue: https://github.com/opensearch-project/opensearch-plugins/issues/10 Feel free to comment on how you'd like to see or suggest naming conventions or add more too :)

saratvemulapalli commented 3 years ago

We could meet somewhere in between which works for everybody. Just another thought. What do you guys think of something like _opendistro/_anomaly_detection/* -> _plugins/_anomaly_detection/*. No more having OpenSearch in it, also we can clearly distinguish that these paths are from plugins.

Same with indices, settings, permissions etc. Please vote 👍🏻 👎🏻 and we can close this down.

cliu123 commented 3 years ago

@cliu123 Do you need a section for class names, method names and variable names in https://github.com/opensearch-project/opensearch-plugins/blob/main/UPGRADING.md?

@dblock @saratvemulapalli Is it possible to make everything as consistent as possible? The index has been named with _. But in the UPGRADE.md, it is kibana --> opensearchDashboards. There're a bunch of static roles, users, settings and configurations containing kibana that are all lowercases with _, for example. I'm about to replace kibana with 'opensearch_dashboards' to follow the naming convention of the index. Is that ok?

saratvemulapalli commented 3 years ago

@cliu123 Do you need a section for class names, method names and variable names in https://github.com/opensearch-project/opensearch-plugins/blob/main/UPGRADING.md?

@dblock @saratvemulapalli Is it possible to make everything as consistent as possible? The index has been named with _. There're a bunch of static roles, users, settings and configurations containing kibana, for example. I'm about to replace kibana with 'opensearch_dashboards' to follow the naming convention of the index. Is that ok?

We will have different conventions for different types. This is how it was done in ODFE as well. All we would want to decide here is that do we want to have as prefix opensearch or plugins etc For example:

  1. APIs will have a prefix of _
  2. Indices will have .
  3. settings will have no prefix. I would prefer to follow them because Elasticsearch/OpenSearch has this naming convention across the code base. We do not want to be different unless there is strong contention.
saratvemulapalli commented 3 years ago

Updating the information from the forum post. We have decided to go with _plugins/_anomaly_detection/*. I'll close this issue and let @dblock take care of the documentation of all the naming convention, its being tracked at: #10

Feel free to re-open if you have concerns or feedback.