Refactor Netty4 method #166

mloufra · 2022-09-30T22:45:05Z

Description

Create a new class GetNetty4Transport.java to handle getNetty4Transport and initializeExtensionTransportService method for refactoring the class in ExtensionsRunner.java.

Issues Resolved

#116

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 <mloufra@amazon.com>

… main

Signed-off-by: mloufra <mloufra@amazon.com>

codecov-commenter · 2022-09-30T23:55:29Z

Codecov Report

Merging #166 (0e9cd6f) into main (7218f6e) will increase coverage by 0.14%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##               main     #166      +/-   ##
============================================
+ Coverage     65.73%   65.87%   +0.14%     
- Complexity       94       95       +1     
============================================
  Files            24       25       +1     
  Lines           464      466       +2     
  Branches         13       13              
============================================
+ Hits            305      307       +2     
  Misses          151      151              
  Partials          8        8

Impacted Files	Coverage Δ
...main/java/org/opensearch/sdk/ExtensionsRunner.java	`63.01% <50.00%> (-4.43%)`	⬇️
...in/java/org/opensearch/sdk/GetNetty4Transport.java	`92.85% <92.85%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

owaiskazi19 · 2022-10-01T00:31:09Z

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

+ * This class initializes a Netty4Transport object and control communication between the extension and OpenSearch.
+ */
+
+public class GetNetty4Transport {


Let's rename it here and everywhere to NettyTransport or Netty4Transport

owaiskazi19 · 2022-10-01T00:33:36Z

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

+    /**
+     * This field is initialized by a call from {@link GetNetty4Transport}.
+     */
+    public final TransportInterceptor NOOP_TRANSPORT_INTERCEPTOR = new TransportInterceptor() {


You can move this to the newly created Netty4Transportclass and make it private.

owaiskazi19 · 2022-10-01T00:33:44Z

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

+    /**
+     * This String get a call from {@link GetNetty4Transport}.
+     */
+    public static final String NODE_NAME_SETTING = "node.name";


You can move this to the newly created Netty4Transport class and make it private.

owaiskazi19 · 2022-10-01T00:35:47Z

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

@@ -170,7 +157,7 @@ private ExtensionsRunner(Extension extension) throws IOException {
        // save custom settings
        this.customSettings = extension.getSettings();
        // initialize the transport service
-        this.initializeExtensionTransportService(this.getSettings());
+        this.getNetty4Transport.initializeExtensionTransportService(this.getSettings(), null);


This method can be called like

getNetty4Transport.initializeExtensionTransportService(....

Why are we passing null here? We should avoid it.

Why are we passing null here? We should avoid it.

The initializeExtensionTransportService method need to call method extensionTransportService from ExtensionsRunner.java, so we should have a reference to the object. And it will be this instead of null. I will change that in next commit.

owaiskazi19 · 2022-10-01T00:41:54Z

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

@@ -591,7 +501,7 @@ public static void main(String[] args) throws IOException {
        ExtensionsRunner extensionsRunner = new ExtensionsRunner();

        // initialize the transport service
-        extensionsRunner.initializeExtensionTransportService(extensionsRunner.getSettings());
+        extensionsRunner.getNetty4Transport.initializeExtensionTransportService(extensionsRunner.getSettings(), extensionsRunner);


We should avoid passing the object of extensionRunner here directly. Instead, all of the required methods, variables should be moved to Netty4Transport file.

owaiskazi19 · 2022-10-01T00:42:46Z

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


    @BeforeEach
    public void setUp() throws IOException {
-        this.extensionsRunner = new ExtensionsRunner();
+        new ExtensionsRunner();


What are we doing here exactly?

Before we start refactor nettyTransport method, we call the nettyTransport method from ExtensionsRunner.java. Since the getNetty4Transport moved to a new class, we do not need to call this method from ExtensionsRunner.java. So I use IDE delete this line, but the IDE do not help delete whole line, half line has been left. I will delete that in next commit.

dbwiddis

As @owaiskazi19 said, we don't need to pass an ExtensionsRunner object to this class, as the only thing we really do with it is set the value of the transport service -- which is returned from the method anyway. I realize the code you're refactoring was already somewhat redundant so it's a bit confusing but this is a good opportunity to clean it up.

dbwiddis · 2022-10-01T02:46:52Z

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

+            NetworkModule.getNamedWriteables().stream(),
+            indicesModule.getNamedWriteables().stream(),
+            searchModule.getNamedWriteables().stream(),
+            null,


No need to include a null in the stream. Just omit it. If there's a reason we are excluding something (I think this is where plugins module goes?) just add a comment explaining why it doesn't look like the other calls.

dbwiddis · 2022-10-01T02:48:26Z

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

+
+        final ConnectionManager connectionManager = new ClusterConnectionManager(settings, transport);
+
+        // Stop any existing transport service


I added this code block while it was in the ExtensionsRunner class. Now that it's in its own class we have much better control over its initialization so you can remove these lines.

dbwiddis · 2022-10-01T02:49:36Z

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

+        }
+
+        // create transport service
+        extensionsRunner.extensionTransportService = new TransportService(


Let's not set a field in another class from here. This method returns the initialized transport service, so just return it and in the ExtensionsRunner where this method is called, just set the value there.

I realize the method in the old class was awkward setting a variable we already had access to, but I think that was for tests. Moving it to this class should make everything much more self-contained.

Signed-off-by: mloufra <mloufra@amazon.com>

dbwiddis

LGTM as a refactor. Consider the suggestions but don't hold up the PR for them.

I still think it's really confusing having the transportService object being referenced from outside the class, but that's a more complicated discussion we can deal with at another time.

dbwiddis · 2022-10-04T17:06:58Z

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

    private ExtensionNamedWriteableRegistry namedWriteableRegistryApi = new ExtensionNamedWriteableRegistry();
    private ExtensionsInitRequestHandler extensionsInitRequestHandler = new ExtensionsInitRequestHandler();
    private OpensearchRequestHandler opensearchRequestHandler = new OpensearchRequestHandler();
    private ExtensionsIndicesModuleRequestHandler extensionsIndicesModuleRequestHandler = new ExtensionsIndicesModuleRequestHandler();
    private ExtensionsIndicesModuleNameRequestHandler extensionsIndicesModuleNameRequestHandler =
        new ExtensionsIndicesModuleNameRequestHandler();
    private ExtensionsRestRequestHandler extensionsRestRequestHandler = new ExtensionsRestRequestHandler();
+    private NettyTransport nettyTransport = new NettyTransport();


(Suggestion) rather than sending this in the initialize call, send this in the constructor here and save it as an instance field so you don't have to pass it later.

dbwiddis · 2022-10-04T17:14:45Z

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

+            emptySet()
+        );
+        extensionsRunner.startTransportService(extensionsRunner.extensionTransportService);
+        return extensionsRunner.extensionTransportService;


(Suggestion) We're never using this return value, could just make this method return void.

mloufra · 2022-10-04T17:20:48Z

LGTM as a refactor. Consider the suggestions but don't hold up the PR for them.

I still think it's really confusing having the transportService object being referenced from outside the class, but that's a more complicated discussion we can deal with at another time.

I will deal with these comment in my next PR.

saratvemulapalli · 2022-10-04T18:17:46Z

LGTM as a refactor. Consider the suggestions but don't hold up the PR for them.

I still think it's really confusing having the transportService object being referenced from outside the class, but that's a more complicated discussion we can deal with at another time.

+1

* issue opensearch-project#28 Signed-off-by: mloufra <mloufra@amazon.com> * Update the lastest coomit Signed-off-by: mloufra <mloufra@amazon.com> * Rename the method and fix the conflict Signed-off-by: mloufra <mloufra@amazon.com> * fix merge conflict Signed-off-by: mloufra <mloufra@amazon.com> * Add code coverage report Signed-off-by: mloufra <mloufra@amazon.com> * Rebase the lastest commit Signed-off-by: mloufra <mloufra@amazon.com> * update the lastest commit Signed-off-by: mloufra <mloufra@amazon.com> * refactor netty4 and initialozaExtensionTransport method Signed-off-by: mloufra <mloufra@amazon.com> * resolve comment Signed-off-by: mloufra <mloufra@amazon.com> Signed-off-by: mloufra <mloufra@amazon.com>

mloufra and others added 14 commits August 26, 2022 16:43

issue #28

7502353

Signed-off-by: mloufra <mloufra@amazon.com>

Update the lastest coomit

dbb47f5

Signed-off-by: mloufra <mloufra@amazon.com>

Rename the method and fix the conflict

bea6873

Signed-off-by: mloufra <mloufra@amazon.com>

fix merge conflict

0e5dd7f

Signed-off-by: mloufra <mloufra@amazon.com>

Add code coverage report

11bd149

Signed-off-by: mloufra <mloufra@amazon.com>

Rebase the lastest commit

bbccd26

Signed-off-by: mloufra <mloufra@amazon.com>

updata the lastest commit

b182b35

Signed-off-by: mloufra <mloufra@amazon.com>

update the lastest commit

1bcb46c

Signed-off-by: mloufra <mloufra@amazon.com>

Merge branch 'opensearch-project:main' into main

50ff5e7

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

2309183

… main

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

05afe09

… main

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

cdac476

… main

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

880001a

… main

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

38e7c45

… main

mloufra requested a review from a team September 30, 2022 22:45

mloufra added 2 commits September 30, 2022 23:35

Merge branch 'main' of https://github.com/mloufra/opensearch-sdk into…

fc39396

… main

refactor netty4 and initialozaExtensionTransport method

0e9cd6f

Signed-off-by: mloufra <mloufra@amazon.com>

owaiskazi19 reviewed Oct 1, 2022

View reviewed changes

dbwiddis reviewed Oct 1, 2022

View reviewed changes

resolve comment

a8f236f

Signed-off-by: mloufra <mloufra@amazon.com>

dbwiddis approved these changes Oct 4, 2022

View reviewed changes

saratvemulapalli approved these changes Oct 4, 2022

View reviewed changes

mloufra merged commit 8822d95 into opensearch-project:main Oct 4, 2022

mloufra deleted the refactor-netty4 branch October 4, 2022 18:40

This was referenced Oct 6, 2022

Refactor transportservice request #167

Merged

Transport service #170

Closed

Transport service #191

Merged

dbwiddis mentioned this pull request Oct 18, 2022

Fix TransportService initialization #199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Netty4 method #166

Refactor Netty4 method #166

mloufra commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022

owaiskazi19 Oct 1, 2022 •

edited

Loading

owaiskazi19 Oct 1, 2022

owaiskazi19 Oct 1, 2022 •

edited

Loading

owaiskazi19 Oct 1, 2022

owaiskazi19 Oct 1, 2022

mloufra Oct 3, 2022 •

edited

Loading

owaiskazi19 Oct 1, 2022

owaiskazi19 Oct 1, 2022

mloufra Oct 3, 2022

dbwiddis left a comment

dbwiddis Oct 1, 2022

dbwiddis Oct 1, 2022

dbwiddis Oct 1, 2022

dbwiddis left a comment

dbwiddis Oct 4, 2022

dbwiddis Oct 4, 2022

mloufra commented Oct 4, 2022

saratvemulapalli commented Oct 4, 2022


		final ConnectionManager connectionManager = new ClusterConnectionManager(settings, transport);

		// Stop any existing transport service

Refactor Netty4 method #166

Refactor Netty4 method #166

Conversation

mloufra commented Sep 30, 2022

Description

Issues Resolved

codecov-commenter commented Sep 30, 2022

Codecov Report

owaiskazi19 Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloufra Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloufra commented Oct 4, 2022

saratvemulapalli commented Oct 4, 2022

owaiskazi19 Oct 1, 2022 •

edited

Loading

owaiskazi19 Oct 1, 2022 •

edited

Loading

mloufra Oct 3, 2022 •

edited

Loading