-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Load a plugin by alias name. #12796
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
Load a plugin by alias name. #12796
Conversation
| addAliasedPlugins(CODECS); | ||
| } | ||
|
|
||
| private static <T> void addAliasedPlugins(Map<String, Class<T>> pluginCache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for review.
Here I had the doubt to use streams, introducing a support class:
private static final class NameAndClass<T> {
final String name;
final Class<T> clazz;
NameAndClass(String name, Class<T> clazz) {
this.name = name;
this.clazz = clazz;
}
@SuppressWarnings("unchecked")
static <T> NameAndClass<T> fromEntry(Map.Entry<String, T> e) {
return new NameAndClass<T>(e.getKey(), (Class<T>) e.getValue());
}
@SuppressWarnings("unchecked")
public <T> NameAndClass<T> withName(String newName) {
return new NameAndClass<T>(newName, (Class<T>) clazz);
}
}
final Map<String, Class<T>> aliasedToAdd = pluginCache.entrySet().stream()
.map(NameAndClass::fromEntry)
.filter(e -> isAliased(e.name))
.map(n -> n.withName(aliasFromOriginal(n.name)))
.filter(n -> !pluginCache.containsKey(n.name))
.collect(Collectors.<NameAndClass, String, Class<T>>toMap(n -> n.name, n -> n.clazz));But we have to create a support class only for this and has a lot of object creation due to stream
…ased plugin and that a pipeline is correcrtly instantiated when an aliased plugin is used
Co-authored-by: Ry Biesemeyer <[email protected]>
Co-authored-by: Ry Biesemeyer <[email protected]>
It generates 2 problems: - a reference overwriting when the aliases plugin is registered before the alias itself, there is an overwriting of an instance already created and possibly used by some other parts of the code, so a dangling reference - a runtime error when a real plugin with same name of the alias is released in the future, due to a variable (resolved_plugin_name) not initialized
Because `Registry#namespace_lookup` will load the _first_ valid plugin it finds, we cannot rely on it finding the concrete class when an alias of the same name exists. Instead we need to revert `Registry#is_a_plugin?` to its original definition so that `namespace_lookup` will only select the specific plugin that has been requested, and introduce a new `Registry#is_a_plugin_or_alias?` to use in the one context where we are willing to accept either.
9587b46 to
1b72aac
Compare
yaauie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've got one last set of changes to suggest, but otherwise LGTM.
I'd like to get this merged/backported and chase down the unrelated DLQ test failures separately.
logstash-core/src/main/java/org/logstash/plugins/discovery/PluginRegistry.java
Outdated
Show resolved
Hide resolved
logstash-core/src/main/java/org/logstash/plugins/discovery/PluginRegistry.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ry Biesemeyer <[email protected]>
Co-authored-by: Ry Biesemeyer <[email protected]>
yaauie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Let's merge & backport as soon as the ruby tests pass CI.
| aliasesToAdd.put(aliasName, e.getValue()); | ||
| final String typeStr = pluginType.name().toLowerCase(); | ||
| LOGGER.info("Plugin {}-{} is aliased as {}-{}", typeStr, realPluginName, typeStr, aliasName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦🏼
| } |
Introcuce the concept of alias for a plugin. Creates an AliasRegistry to map plugin aliases to original plugins. If a real plugin with same name of the an alias is present in the system, then the real plugin take precedence during the instantiation of the pipeline. Simplified the error handling in class lookup Co-authored-by: Ry Biesemeyer <[email protected]>
Introcuce the concept of alias for a plugin (#12796) and removes static part from PluginRegistry to avoid static initializer (#12799) Creates an AliasRegistry to map plugin aliases to original plugins. If a real plugin with same name of the an alias is present in the system, then the real plugin take precedence during the instantiation of the pipeline. Simplified the error handling in class lookup Co-authored-by: Ry Biesemeyer <[email protected]>
* master: fix DLQ integration tests (elastic#12871) Fix ES HOW integration tests on `master` (elastic#12872) Update logstash_releases.json Support for UTF-16 and other multi-byte-character logfiles (elastic#9702) change download path for geoip plugin (elastic#12863) Change Gradle's :logstash-integration-tests:integrationTests task to depends on copyES (elastic#12847) Doc: Keystore must be accessible to logstash user (elastic#12775) Remove json ~>1 pinning (elastic#12851) Adapted install/uninstall/list PluginManager's command to respect the alised plugins (elastic#12821) Load a plugin by alias name. (elastic#12796)
If the plugin name is an alias and no other plugin exists with same name then load the alised one
Release notes
Introduce the concept of aliased plugin, to let a plugin be renamed easily
What does this PR do?
An alias for a plugin is a just a rename of an existing plugin, in this way the alias can be used in pipeline configuration. If later a real plugin with same name of the alias is released then that plugin takes precedence over the alias name.
Suppose we a plugin filter named
teleportand we also alias it asteleport_over_fiber, theteleport_over_fiberalias can be used in interchangeable way asteleportwith same parameters and behavior. In a future when a real plugin that leverage fiber transmission for teleport a pluginteleport_over_fibername is effectively released, this real plugin takes precedence to over the alias.The changes to
logstash-plugintool used to manage the installation, listing and removal of aliased plugin is handled by the PR #12821Why is it important/What is the impact to the user?
It lets the user to star using today plugins names that will be released as independent plugin in near future.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Use an alias in a pipeline config, for example:
check that it runs correctly with
bin/logstash -f pipeline.confRelated issues
Use cases
Screenshots
Logs