New Design with createComponent extension point #8

owaiskazi19 · 2022-05-09T22:49:45Z

Signed-off-by: Owais Kazi [email protected]

Description

Part of opensearch-project/OpenSearch#2981
Also, resolves #6

Note: Build will fail as this PR has dependency on opensearch-project/OpenSearch#3265.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

src/main/java/org/opensearch/sdk/ClusterStateResponseHandler.java

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

src/main/java/org/opensearch/sdk/LocalNodeResponseHandler.java

src/main/java/org/opensearch/sdk/ClusterSettingResponseHandler.java

src/main/java/org/opensearch/sdk/ClusterStateResponseHandler.java

src/main/java/org/opensearch/sdk/ClusterSettingResponseHandler.java

dbwiddis · 2022-06-15T04:20:35Z

src/main/java/org/opensearch/sdk/ClusterSettingResponseHandler.java

+
+public class ClusterSettingResponseHandler implements TransportResponseHandler<ClusterSettingsResponse> {
+    private static final Logger logger = LogManager.getLogger(ClusterSettingResponseHandler.class);
+    final CountDownLatch inProgressLatch = new CountDownLatch(1);


Does this need to be package private to be read outside this class? If not, it should be private (same question/comment elsewhere in this PR.)

What is the purpose of this object, it doesn't seem to be impacted flow of control? Here is a stack overflow answer about CountDownLatch's usage.

#8 (comment)

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

dbwiddis · 2022-06-15T04:39:39Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

    private static ExtensionSettings extensionSettings = null;
+    private DiscoveryNode opensearchNode = null;
+    TransportService transportService = null;


We are still using a TransportService argument in other methods like startTransportService(). I see in this comment it's "available when needed" but there is no getter for it and we are not ever using it.

It happens to be "available" package private, but is that intentional? Given that you've added a public setter for it, should it be private with a getter since otherwise we're setting it and forgetting it (other than perhaps the reference to it from a reachability perspective)?

Also, this same variable name is used as a local variable later in createTransportService() and main(). That's not a good idea.

Good call! Made transportService private and created a getter for the same.

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

src/main/java/org/opensearch/sdk/ExtensionClusterStateResponseHandler.java

dbwiddis

TLDR: I'm not sure what the right answer is with TransportService, but I don't think what we have is it.

That pesky TransportService instance variable needs a bit more careful thought and handling here. You're both setting it and returning it with the create() method, and elsewhere using both effects (return and instance) interchangeably.

If we need to keep the same API (with a return value from create*() and an argument to start*()) then we should find a more consistent way to work within those constraints (perhaps not have the instance variable and require the user to create and then pass it).

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

Signed-off-by: Owais Kazi <[email protected]>

saratvemulapalli

I did one pass, tiny comments so far.

saratvemulapalli · 2022-06-21T03:56:52Z

src/main/java/org/opensearch/sdk/ClusterSettingsResponseHandler.java

+
+    @Override
+    public void handleResponse(ClusterSettingsResponse response) {
+        logger.info("received {}", response);


Lets make this log at debug level?

src/main/java/org/opensearch/sdk/ClusterSettingsResponseHandler.java

src/main/java/org/opensearch/sdk/ExtensionClusterStateResponseHandler.java

src/test/java/org/opensearch/sdk/TestMainScript.java

saratvemulapalli

WIth adding API support for createComponent, can you add unit tests ?

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

src/main/java/org/opensearch/sdk/LocalNodeResponseHandler.java

owaiskazi19 · 2022-06-24T00:28:13Z

WIth adding API support for createComponent, can you add unit tests ?

Abstracted into APIs. Will add tests together as a part of #25

Signed-off-by: Owais Kazi <[email protected]>

saratvemulapalli

Thanks for taking care of the comments.
Looks like CI is failing, like due to change not merged to feature/extensions branch on OpenSearch?

owaiskazi19 · 2022-06-27T17:56:25Z

Thanks for taking care of the comments. Looks like CI is failing, like due to change not merged to feature/extensions branch on OpenSearch?

Yes. Build will fail as this PR has dependency on opensearch-project/OpenSearch#3265.

Update: PR got merged and the build passed.

dbwiddis

Approved but I really want some placeholder (comment, TODO, log warning, something) about making sure the opensearchNode isn't null before using it. Go ahead and add as an addition for a later issue if needed, just don't want it forgotten.

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

Signed-off-by: Owais Kazi <[email protected]>

* Draft createComponent extension point Signed-off-by: Owais Kazi <[email protected]> * Resolved issue of transport stopped Signed-off-by: Owais Kazi <[email protected]> * Decoupled response handler to separate classes Signed-off-by: Owais Kazi <[email protected]> * Addressed PR Comments Signed-off-by: Owais Kazi <[email protected]> * Removed CountDown Latch Signed-off-by: Owais Kazi <[email protected]> * Refactoring and PR comments Signed-off-by: Owais Kazi <[email protected]> * Addressed PR Comments Signed-off-by: Owais Kazi <[email protected]> * Abstracted send request to APIs Signed-off-by: Owais Kazi <[email protected]> * Added comment for OpenSearchNode Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the draft-createComponent branch from 63c65f2 to c1070df Compare May 31, 2022 18:58

owaiskazi19 marked this pull request as ready for review May 31, 2022 21:49

owaiskazi19 requested a review from a team May 31, 2022 21:49

owaiskazi19 changed the title ~~Draft createComponent extension point~~ createComponent extension point May 31, 2022

joshpalis previously approved these changes Jun 1, 2022

View reviewed changes

owaiskazi19 requested review from saratvemulapalli and ryanbogan June 1, 2022 21:15

owaiskazi19 self-assigned this Jun 1, 2022

owaiskazi19 changed the title ~~createComponent extension point~~ New Design with createComponent extension point Jun 10, 2022

peternied reviewed Jun 13, 2022

View reviewed changes

dbwiddis reviewed Jun 14, 2022

View reviewed changes

owaiskazi19 force-pushed the draft-createComponent branch from dca68bc to 102a416 Compare June 14, 2022 21:30

owaiskazi19 requested review from peternied and dbwiddis June 15, 2022 00:13

dbwiddis requested changes Jun 15, 2022

View reviewed changes

owaiskazi19 requested a review from dbwiddis June 17, 2022 00:13

dbwiddis requested changes Jun 17, 2022

View reviewed changes

owaiskazi19 requested a review from dbwiddis June 21, 2022 21:22

owaiskazi19 commented Jun 21, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved

owaiskazi19 added 7 commits June 21, 2022 14:52

Draft createComponent extension point

937af48

Signed-off-by: Owais Kazi <[email protected]>

Resolved issue of transport stopped

c4c6f89

Signed-off-by: Owais Kazi <[email protected]>

Decoupled response handler to separate classes

2819c90

Signed-off-by: Owais Kazi <[email protected]>

Addressed PR Comments

267ed2d

Signed-off-by: Owais Kazi <[email protected]>

Removed CountDown Latch

3dca183

Signed-off-by: Owais Kazi <[email protected]>

Refactoring and PR comments

1a6a502

Signed-off-by: Owais Kazi <[email protected]>

Addressed PR Comments

ebeda8a

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the draft-createComponent branch from a47512e to ebeda8a Compare June 21, 2022 21:53

joshpalis self-requested a review June 22, 2022 22:43

saratvemulapalli reviewed Jun 23, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/sdk/LocalNodeResponseHandler.java Show resolved Hide resolved

Abstracted send request to APIs

ba1ec1e

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the draft-createComponent branch from e74de57 to ba1ec1e Compare June 24, 2022 00:32

owaiskazi19 requested a review from saratvemulapalli June 24, 2022 00:32

saratvemulapalli previously approved these changes Jun 27, 2022

View reviewed changes

dbwiddis previously approved these changes Jun 29, 2022

View reviewed changes

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved

Added comment for OpenSearchNode

8335be7

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 dismissed stale reviews from dbwiddis, saratvemulapalli, and joshpalis via 8335be7 June 29, 2022 17:26

dbwiddis approved these changes Jun 29, 2022

View reviewed changes

saratvemulapalli approved these changes Jun 29, 2022

View reviewed changes

owaiskazi19 merged commit c825b83 into opensearch-project:main Jun 29, 2022

joshpalis approved these changes Jun 29, 2022

View reviewed changes

dbwiddis mentioned this pull request Jul 12, 2022

Fail build on missing Javadocs #45

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Design with createComponent extension point #8

New Design with createComponent extension point #8

owaiskazi19 commented May 9, 2022 •

edited

Loading

dbwiddis Jun 15, 2022

peternied Jun 15, 2022

owaiskazi19 Jun 16, 2022

dbwiddis Jun 15, 2022 •

edited

Loading

owaiskazi19 Jun 16, 2022

dbwiddis left a comment •

edited

Loading

saratvemulapalli left a comment

saratvemulapalli Jun 21, 2022

saratvemulapalli left a comment

owaiskazi19 commented Jun 24, 2022

saratvemulapalli left a comment

owaiskazi19 commented Jun 27, 2022 •

edited

Loading

dbwiddis left a comment

New Design with createComponent extension point #8

New Design with createComponent extension point #8

Conversation

owaiskazi19 commented May 9, 2022 • edited Loading

Description

Issues Resolved

dbwiddis Jun 15, 2022

Choose a reason for hiding this comment

peternied Jun 15, 2022

Choose a reason for hiding this comment

owaiskazi19 Jun 16, 2022

Choose a reason for hiding this comment

dbwiddis Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

owaiskazi19 Jun 16, 2022

Choose a reason for hiding this comment

dbwiddis left a comment • edited Loading

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

saratvemulapalli Jun 21, 2022

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

owaiskazi19 commented Jun 24, 2022

saratvemulapalli left a comment

Choose a reason for hiding this comment

owaiskazi19 commented Jun 27, 2022 • edited Loading

dbwiddis left a comment

Choose a reason for hiding this comment

owaiskazi19 commented May 9, 2022 •

edited

Loading

dbwiddis Jun 15, 2022 •

edited

Loading

dbwiddis left a comment •

edited

Loading

owaiskazi19 commented Jun 27, 2022 •

edited

Loading