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.

Sign up for GitHub

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

Refactor ExtensionsRunner test code out of production source tree #172

Merged
merged 2 commits into from
Oct 10, 2022
Merged

Refactor ExtensionsRunner test code out of production source tree #172

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b34714c
Refactor out test-only extension runner contructor
roulpriya Oct 10, 2022
1cb21b5
Merge branch 'main' into Issue-171
dbwiddis Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/main/java/org/opensearch/sdk/ExtensionSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ public class ExtensionSettings {
private String hostAddress;
private String hostPort;

/**
* Path to extension.yml file for testing. Internal use only.
*/
static final String EXTENSION_DESCRIPTOR = "src/test/resources/extension.yml";

/**
* Jackson requires a default constructor.
*/
Expand Down
47 changes: 2 additions & 45 deletions src/main/java/org/opensearch/sdk/ExtensionsRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
*/
package org.opensearch.sdk;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.cluster.node.DiscoveryNode;
Expand Down Expand Up @@ -50,10 +47,8 @@
import org.opensearch.transport.TransportService;
import org.opensearch.transport.TransportSettings;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -114,28 +109,13 @@ public class ExtensionsRunner {
*/
UpdateSettingsRequestHandler updateSettingsRequestHandler = new UpdateSettingsRequestHandler();

/**
* Instantiates a new Extensions Runner using test settings.
*
* @throws IOException if the runner failed to read settings or API.
*/
public ExtensionsRunner() throws IOException {
ExtensionSettings extensionSettings = readExtensionSettings();
this.settings = Settings.builder()
.put(NODE_NAME_SETTING, extensionSettings.getExtensionName())
.put(TransportSettings.BIND_HOST.getKey(), extensionSettings.getHostAddress())
.put(TransportSettings.PORT.getKey(), extensionSettings.getHostPort())
.build();
this.customSettings = Collections.emptyList();
}

/**
* Instantiates a new Extensions Runner using the specified extension.
*
* @param extension The settings with which to start the runner.
* @throws IOException if the runner failed to read settings or API.
*/
private ExtensionsRunner(Extension extension) throws IOException {
protected ExtensionsRunner(Extension extension) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Note to self/other reviewers: package private would be sufficient for tests if we only want to make ExtensionsRunner extendable for testing. However I think it's reasonable to think any user might wish to create a simple anonymous extension (possibly for their own testing!) so I like using protected here.

ExtensionSettings extensionSettings = extension.getExtensionSettings();
this.settings = Settings.builder()
.put(NODE_NAME_SETTING, extensionSettings.getExtensionName())
Expand All @@ -152,14 +132,6 @@ private ExtensionsRunner(Extension extension) throws IOException {
this.customSettings = extension.getSettings();
// initialize the transport service
nettyTransport.initializeExtensionTransportService(this.getSettings(), this);
// start listening on configured port and wait for connection from OpenSearch
this.startActionListener(0);
}

private static ExtensionSettings readExtensionSettings() throws IOException {
File file = new File(ExtensionSettings.EXTENSION_DESCRIPTOR);
ObjectMapper objectMapper = new ObjectMapper(new YAMLFactory());
return objectMapper.readValue(file, ExtensionSettings.class);
}

/**
Expand Down Expand Up @@ -478,21 +450,6 @@ public static void run(Extension extension) throws IOException {
logger.info("Starting extension " + extension.getExtensionSettings().getExtensionName());
@SuppressWarnings("unused")
ExtensionsRunner runner = new ExtensionsRunner(extension);
}

/**
* Run the Extension. For internal/testing purposes only. Imports settings and sets up Transport Service listening for incoming connections.
*
* @param args Unused
* @throws IOException if the runner failed to connect to the OpenSearch cluster.
*/
public static void main(String[] args) throws IOException {

ExtensionsRunner extensionsRunner = new ExtensionsRunner();

// initialize the transport service
extensionsRunner.nettyTransport.initializeExtensionTransportService(extensionsRunner.getSettings(), extensionsRunner);
// start listening on configured port and wait for connection from OpenSearch
extensionsRunner.startActionListener(0);
runner.startActionListener(0);
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/sdk/TransportActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void sendRegisterTransportActionsRequest(TransportService transportServic
transportService.sendRequest(
opensearchNode,
ExtensionsOrchestrator.REQUEST_EXTENSION_REGISTER_TRANSPORT_ACTIONS,
new RegisterTransportActionsRequest(transportActions),
new RegisterTransportActionsRequest("", transportActions),
dbwiddis marked this conversation as resolved.
Show resolved Hide resolved
registerTransportActionsResponseHandler
);
} catch (Exception e) {
Expand Down
32 changes: 32 additions & 0 deletions src/test/java/org/opensearch/sdk/ExtensionsRunnerForTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package org.opensearch.sdk;
Copy link
Member

Choose a reason for hiding this comment

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

This is great!
It sets up our initial framework for tests going forward.


import java.io.IOException;
import java.util.Collections;
import java.util.List;

/**
* An Extension Runner for testing using test settings.
*/
public class ExtensionsRunnerForTest extends ExtensionsRunner {

/**
* Instantiates a new Extensions Runner using test settings.
*
* @throws IOException if the runner failed to read settings or API.
*/
public ExtensionsRunnerForTest() throws IOException {
super(new Extension() {
@Override
public ExtensionSettings getExtensionSettings() {
return new ExtensionSettings("sample-extension", "127.0.0.1", "4532");
}

@Override
public List<ExtensionRestHandler> getExtensionRestHandlers() {
return Collections.emptyList();
}
});

}

}
4 changes: 2 additions & 2 deletions src/test/java/org/opensearch/sdk/TestExtensionSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
import java.io.File;

public class TestExtensionSettings extends OpenSearchTestCase {

private static final String EXTENSION_DESCRIPTOR = "src/test/resources/extension.yml";
private ExtensionSettings extensionSettings;

@Override
@BeforeEach
public void setUp() throws Exception {
super.setUp();
File file = new File(ExtensionSettings.EXTENSION_DESCRIPTOR);
File file = new File(EXTENSION_DESCRIPTOR);
ObjectMapper objectMapper = new ObjectMapper(new YAMLFactory());
extensionSettings = objectMapper.readValue(file, ExtensionSettings.class);
}
Expand Down
17 changes: 16 additions & 1 deletion src/test/java/org/opensearch/sdk/TestExtensionsRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.HashMap;
import java.util.List;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.opensearch.Version;
Expand Down Expand Up @@ -77,11 +78,14 @@ public class TestExtensionsRunner extends OpenSearchTestCase {
private ExtensionsRunner extensionsRunner;
private TransportService transportService;

private TransportService initialTransportService;
owaiskazi19 marked this conversation as resolved.
Show resolved Hide resolved

@Override
@BeforeEach
public void setUp() throws Exception {
super.setUp();
this.extensionsRunner = new ExtensionsRunner();
this.extensionsRunner = new ExtensionsRunnerForTest();
dbwiddis marked this conversation as resolved.
Show resolved Hide resolved
this.initialTransportService = extensionsRunner.extensionTransportService;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constructor of ExtensionRunner creates an instance of TransportService which listens a port in the background.

Tests further use setExtensionTransportService to replace the created instance with a mock to use in testing. The original instance becomes unreachable and our tearDown cannot stop it, which causes the rest of the tests to fail due to the port being already in use. Hence, this hack.

IMO, we should pass TransportService as a constructor parameter and not instantiate inside.

Copy link
Member

Choose a reason for hiding this comment

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

At this point, let's "hack" as needed to get this merged.

I agree with you and have questioned how we've tracked the transport service from some of the initial PRs. I'm not sure putting it in the constructor is the best solution but we do need something better than what we have.

We do have a future requirement to gracefully handle disconnections/reconnections (hot plugin, etc.) and I think handling these will definitely need to be part of that functionality as well.

this.transportService = spy(
new TransportService(
Settings.EMPTY,
Expand All @@ -95,6 +99,17 @@ public void setUp() throws Exception {
);
}

@Override
@AfterEach
public void tearDown() throws Exception {
super.tearDown();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the tearDown. This will definitely avoid the bind connection error.

if (initialTransportService != null) {
this.initialTransportService.stop();
this.initialTransportService.close();
Thread.sleep(1000);
}
}

// test manager method invokes start on transport service
@Test
public void testTransportServiceStarted() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/sdk/TransportCommunicationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void run() {
private void startTransportandClient(Settings settings, Thread client) throws IOException, InterruptedException {

// retrieve transport service
ExtensionsRunner extensionsRunner = new ExtensionsRunner();
ExtensionsRunner extensionsRunner = new ExtensionsRunnerForTest();
owaiskazi19 marked this conversation as resolved.
Show resolved Hide resolved
// start transport service
TransportService transportService = nettyTransport.initializeExtensionTransportService(settings, extensionsRunner);

Expand Down