[FEATURE] Refactor ExtensionsRunner class

dbwiddis commented 2 years ago

Is your feature request related to a problem?

ExtensionsRunner is getting big, could we compartmentalize this functionality? Maybe there is a pattern like configuration/registration/request handling?

_Originally posted by @peternied in https://github.com/opensearch-project/opensearch-sdk-java/pull/109#discussion_r958898763_

What solution would you like?

There are 513 lines.

Up to line 156 is headers, constructors, initializers, getters, setters, etc.
Line 157-244 is handler methods that are registered in lines 328-390
Line 246-390 is initializing the transport service and registering the above request handlers
Line 392-471 is sending transport requests
Line 473-513 is back to setters and startup methods

We can definitely separate out lines 157-471 into at least one other class, possibly two or three.

each of the handlers in lines 157-244 could probably have its own class.
~the transportService.registerRequestHandler() calls are all the same except for two lines, and could benefit from a helper method for the 80-90% of that code that is common.~ (these are all single method calls but take up several lines, probably ok to leave).
The Netty4 and initialize transport service can probably be moved to their own class. (Speaking of netty4, there are some old third party license files that can be removed).

What alternatives have you considered?

Doing nothing. It's tempting.

Do you have any additional context?

Like most code, this starts small and grows to the point where the problem is obvious. We're there.

saratvemulapalli commented 2 years ago

What alternatives have you considered? Doing nothing. It's tempting.

😆

mloufra commented 2 years ago

We can definitely separate out lines 157-471 into at least one other class, possibly two or three.

each of the handlers in lines 157-244 could probably have its own class.

~the transportService.registerRequestHandler() calls are all the same except for two lines, and could benefit from a helper method for the 80-90% of that code that is common.~ (these are all single method calls but take up several lines, probably ok to leave).

The Netty4 and initialize transport service can probably be moved to their own class. (Speaking of netty4, there are some old third party license files that can be removed).

The following is plan to refactor class for ExtensionsRunner

[x] - Having a new class to handle each handler method in line 157-244
[x] - Having a new class to handle Netty4 and initialize transport service method in line 252-327
[ ] - Creating a new method to handle the duplicate code for transport request method in line 398-477

dbwiddis commented 2 years ago

The transport requests all follow the model, so this code could be extracted:

public void sendFooRequest(TransportService transportService) {
    logger.info("Sending " + foo + " request to OpenSearch");
    TransportResponseHandler<Foo> handler = new FooHandler();
    try {
        transportService.sendRequest(
            opensearchNode,
            ExtensionsOrchestrator.REQUEST_FOO,
            new ExtensionRequest(ExtensionsOrchestrator.RequestType.REQUEST_FOO),
            fooResponseHandler
        );
    } catch (Exception e) {
        logger.info("Failed to send " + foo + " request to OpenSearch", e);
    }
}

We could create a new class and instantiate it with transportService and opensearchNode in its constructor.

Then we could create a sendExtensionRequest method which takes as parameters:

String for the request type
the enum for the Request
the Response handler
possibly might need the class type for the <T> needed in the TransportResponseHandler.

mloufra commented 2 years ago

The transport requests all follow the model, so this code could be extracted:
public void sendFooRequest(TransportService transportService) {
    logger.info("Sending " + foo + " request to OpenSearch");
    TransportResponseHandler<Foo> handler = new FooHandler();
    try {
        transportService.sendRequest(
            opensearchNode,
            ExtensionsOrchestrator.REQUEST_FOO,
            new ExtensionRequest(ExtensionsOrchestrator.RequestType.REQUEST_FOO),
            fooResponseHandler
        );
    } catch (Exception e) {
        logger.info("Failed to send " + foo + " request to OpenSearch", e);
    }
}
We could create a new class and instantiate it with transportService and opensearchNode in its constructor.

Then we could create a sendExtensionRequest method which takes as parameters:

String for the request type

the enum for the Request

the Response handler

possibly might need the class type for the <T> needed in the TransportResponseHandler.

@dbwiddis Let me clarification to make sure whether my understanding for this comment is correct or not. What will be done is that moving the transportService method (startTransportService sendRegisterRestActionsRequest sendClusterStateRequest sendClusterSettingsRequest sendLocalNodeRequest) and opensearchNode method (setOpensearchNode getOpensearchNode) to new folder called transport.

Then create a new method based on the example code which shows on comment to make connect between transportService class and ExtensionsRunner class.

For the transportService method, do I need to separate these method in different class?

dbwiddis commented 2 years ago

What will be done is that moving the transportService method (startTransportService sendRegisterRestActionsRequest sendClusterStateRequest sendClusterSettingsRequest sendLocalNodeRequest) and opensearchNode method (setOpensearchNode getOpensearchNode) to new folder called transport.

Then create a new method based on the example code which shows on comment to make connect between transportService class and ExtensionsRunner class.

@mloufra not quite. Let me go back to the "why" of this refactoring. When you see the same code repeated a lot it's good to put the repetitive stuff in one location so there's only a single method to test/debug. Lots of bugs slip in during copy/paste code where someone forgot to change one of the things they copied (for example, the log message on failure for the sendLocalNodeRequest method was copied from the sendClusterSettingsRequest method, and not changed! Minor bug with logging but causes bigger issues when it's logic-impacting code.)

In this case, the "interesting" and "different" part of the code is in the embedded transportService.sendRequest() method calls. The rest of these methods is just repetition of logging and exception handling.

So the goal here is to create a single "helper" method somewhere (either in ExtensionsRunner, in an existing class related to the TransportService, or in a new class you create) that does the logging and exception handling parts. Then you can have just a single method call for each type in ExtensionsRunner passing the objects that are different to that same helper method.

mloufra commented 2 years ago

Thanks @dbwiddis for your clarification. I will update that on my plan.

mloufra commented 2 years ago

Due to delays caused by too frequent merge conflicts, I raise a PR for first step of my plan. After this PR getting merged, I will work on second step of my plan(to create new class handle Netty4 and initialize transport service method).

opensearch-project / opensearch-sdk-java