Skip to content

[FEATURE] Only send one extension's information in InitializeExtensionsRequest #93

@dbwiddis

Description

@dbwiddis

Is your feature request related to a problem?

Currently, the ExtensionsOrchestrator iterates over all extensions when initializing, passing the DiscoveryNode representing a single extension:

public void extensionsInitialize() {
    for (DiscoveryNode extensionNode : extensionsList) {
        extensionInitialize(extensionNode);
    }
}

However, inside this method, rather than using just the extension being initialized, (a copy of) the full extensionsList is passed as a transport parameter:

transportService.sendRequest(
    extensionNode,
    REQUEST_EXTENSION_ACTION_NAME,
    new InitializeExtensionsRequest(transportService.getLocalNode(), new ArrayList<DiscoveryExtension>(extensionsList)),
    extensionResponseHandler
);

In order to obtain information from that List, the extension is forced to iterate over the entire list to match the name as present in the extension to get any of the configuration information.

for (DiscoveryExtension de : extensionInitRequest.getExtensions()) {
    if (de.getName().equals(extensionSettings.getExtensionName())) {
        setUniqueId(de.getId());
        break;
    }
}

This results in needless list iteration, copying, serializing/deserializing, and sending excess information between nodes, along with introducing the need for exception handling in the case there is a name mismatch between the name configured in the list of extensions and the name configured in the extension itself.

What solution would you like?

Change the second argument in the constructor for InitializeExtensionsRequest from a List<DiscoveryExtension> to a single DiscoveryExtension object, and pass that argument during initialization. (The initialization loop should iterate over DiscoveryExtension rather than the DiscoveryNode superclass).

This will require a decision on which extension name configuration takes precedence (or removal of one of the duplicate configs).

What alternatives have you considered?

  1. Leaving it as is: wasteful of CPU and network bandwidth
  2. Having a common extension list in a separate file and only passing a unique key to all methods that need to gain information for it: possibly a better solution but more complex and error-prone than the simple change proposed.

Do you have any additional context?

See #74 (comment)

It only occurs in the case there's a misconfiguration where the extension name in extensions.yml doesn't match the extension name defined by the extension, so terminating the extension on a runtime exception is appropriate.

Which I might argue is an unnecessary requirement and one of the two names should be ignored. However by passing the entire list of DiscoveryExtensions matching the names is the only reliable way to fetch the unique ID. (Maybe we should have the extension itself declare the uniqueId?)

I think the correct thing to do is not bother with the name check, but change PluginRequest (which should be renamed) to pass a single instance of the correct DiscoveryExtension instead of the list.

See also #52 which this may solve.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions