Skip to content
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

[Feature/extensions] Add getNamedWriteableRegistry() API for extensions #291

Closed
ryanbogan opened this issue Dec 16, 2022 · 7 comments
Closed
Assignees
Labels
CCI Part of the College Contributor Initiative enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ryanbogan
Copy link
Member

The old model for the getNamedWriteableRegistry() was recently removed, due to concerns here: opensearch-project/OpenSearch#5518 (comment). A new model must be created, which should be similar to the implementation of getNamedXContent().

@owaiskazi19
Copy link
Member

@ryanbogan this issue should be in #opensearch-sdk

@ryanbogan ryanbogan transferred this issue from opensearch-project/OpenSearch Dec 16, 2022
@dbwiddis
Copy link
Member

@ryanbogan this looks like it's already done here:

List<NamedWriteableRegistry.Entry> namedWriteables = Stream.of(
NetworkModule.getNamedWriteables().stream(),
indicesModule.getNamedWriteables().stream(),
searchModule.getNamedWriteables().stream(),
ClusterModule.getNamedWriteables().stream()
).flatMap(Function.identity()).collect(Collectors.toList());
final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(namedWriteables);

Is there still work to be done on this?

@dbwiddis
Copy link
Member

Ah, I see we create it but we don't integrate the portion of it that comes from an extension itself. SO that bit needs to be added.

@dbwiddis dbwiddis added good first issue Good for newcomers help wanted Extra attention is needed labels Jan 10, 2023
@petiveriaalliacea
Copy link
Contributor

Hello! I want to take this issue. Where can I find nessesary classes and methods? And in what part here is missing and should be implemented?

@dbwiddis
Copy link
Member

dbwiddis commented Mar 6, 2023

Hi, @petiveriaalliacea! Thanks for stepping up.

Take a look at how the getNamedXContent() extension point is currently implemented with the ExtensionsRunner constructor, ExtensionsInitRequestHandler, and SDKNamedXContentRegistry.

You will basically be doing the exact same thing with the getNamedWriteables() extension point. I've deleted the steps here and updated them in the next comment.

Let us know if you have any questions.

@dbwiddis
Copy link
Member

dbwiddis commented Mar 6, 2023

Actually, @petiveriaalliacea it's slightly more (and less) complicated than the above.

  • I realized reading this comment that we already have a partial implementation in the NettyTransport class. So that class needs a copy of this registry.
  • I looked into the registration in Node.java linked in the comment above and none of it depends on updated settings, so it's okay to initialize this once.

So updated steps:
You will basically be doing the exact same thing with the getNamedWriteables() extension point:

  1. Create a new SDKNamedWriteableRegistry class similar to the SDKNamedXContentRegistry, except you won't need an update method
  2. Internally in that class, instead of the OpenSearch module static NamedXContent, use the NamedWriteable implementations from here https://github.com/opensearch-project/OpenSearch/blob/f332977666f9839b5bfa507f9ffe77e8134d2481/server/src/main/java/org/opensearch/node/Node.java#L599 . You can call the static method on one, and instantiate the class to call the getter on the others. This was done in the NettyTransport class but you'll want to move it into that new class.
  3. Delete the creation of this registry from the NettyTransport class. Instead, add the registry as a parameter to the getNetty4Transport method.
  4. Add the default method to the Extension interface
  5. Create a private field in the ExtensionsRunner for the SDKNamedWriteableRegistry with a getter
  6. During initialization, instantiate this registry prior to injecting components
  7. Add the registry to the components injected in Guice (just use the getter from the SDK registry to inject the actual NamedWriteableRegistry)
  8. During transport initialization, pass the registry to the NettyTransport method you changed in step 3.

petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 13, 2023
…ct#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 17, 2023
…ct#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 17, 2023
…ct#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 19, 2023
…ct#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 20, 2023
…nsearch-project#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
@dbwiddis dbwiddis added the CCI Part of the College Contributor Initiative label Mar 24, 2023
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 24, 2023
…nsearch-project#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 24, 2023
…nsearch-project#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 25, 2023
…nsearch-project#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
petiveriaalliacea pushed a commit to petiveriaalliacea/opensearch-sdk-java that referenced this issue Mar 25, 2023
…nsearch-project#291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
dbwiddis pushed a commit that referenced this issue Mar 27, 2023
* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

---------

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Signed-off-by: petiveriaalliacea <77691894+petiveriaalliacea@users.noreply.github.com>
Co-authored-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Co-authored-by: Ryan Bogan <10944539+ryanbogan@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this issue Mar 27, 2023
* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Created ScriptExtension interface #318

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>

---------

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Signed-off-by: petiveriaalliacea <77691894+petiveriaalliacea@users.noreply.github.com>
Co-authored-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Co-authored-by: Ryan Bogan <10944539+ryanbogan@users.noreply.github.com>
(cherry picked from commit e13aed8)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dbwiddis pushed a commit that referenced this issue Mar 27, 2023
* Created ScriptExtension interface #318



* Created ScriptExtension interface #318



* Created ScriptExtension interface #318



* Created ScriptExtension interface #318



* Added getNamedWriteableRegistry() API for extensions #291



* Added getNamedWriteableRegistry() API for extensions #291



* Created ScriptExtension interface #318



* Created ScriptExtension interface #318



* Added getNamedWriteableRegistry() API for extensions #291



* Added getNamedWriteableRegistry() API for extensions #291



* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291



* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291



* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291



* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291



* Fixed Tests. Added getNamedWriteableRegistry() API for extensions #291



---------





(cherry picked from commit e13aed8)

Signed-off-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Signed-off-by: petiveriaalliacea <77691894+petiveriaalliacea@users.noreply.github.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Aisara Imangaliyeva <imangaliyeva.aisara@gmail.com>
Co-authored-by: Ryan Bogan <10944539+ryanbogan@users.noreply.github.com>
@dbwiddis
Copy link
Member

Done in #553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCI Part of the College Contributor Initiative enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants