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]Enforce type safety for NamedWriteableRegistryParseRequest #4923

Merged

Conversation

mloufra
Copy link
Contributor

@mloufra mloufra commented Oct 25, 2022

Signed-off-by: mloufra [email protected]

Description

Add generic type for categoryClass in NamedWriteable RegistryParseRequest, NamedWriteableRegistryResponse, ExtensionNamedWriteableRegistry, NamedWriteableRegistryResponseHandler and ExtensionsOrchestratorTests
Equivalent PR on OpenSearch-sdk-java: opensearch-project/opensearch-sdk-java#203

Issues Resolved

opensearch-project/opensearch-sdk-java#130

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: mloufra <[email protected]>
Signed-off-by: mloufra <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment about placement of the warning suppression.

Good job with all these changes, it was a bit more complicated than I expected!

this.registry = new HashMap<>(registry);
}

/**
* @param in StreamInput from which map entries of writeable names and their associated category classes are read from
* @throws IllegalArgumentException if the fully qualified class name is invalid and the class object cannot be generated at runtime
*/
@SuppressWarnings("unchecked")
Copy link
Member

Choose a reason for hiding this comment

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

Generally we try to put @SuppressWarnings at the lowest scope possible. In the constructor we were assigning a value to an instance variable, so we had to use the method level, but here we are assigning to a local variable. You should be able to put this suppression down above line 47.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, I will move @SuppressWarnings in next commit

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

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.

3 participants