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.

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

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

Closed
dbwiddis opened this issue Aug 16, 2022 · 0 comments · Fixed by #103
Closed

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

dbwiddis opened this issue Aug 16, 2022 · 0 comments · Fixed by #103
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Aug 16, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant