Enable testing for ExtensiblePlugins using classpath plugins #16908
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
|
❌ Gradle check result for 4176c46: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
❌ Gradle check result for 7ddd9a3: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 7ddd9a3: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
server/src/main/java/org/opensearch/plugins/PluginsService.java
Outdated
Show resolved
Hide resolved
| final Collection<Class<? extends Plugin>> classpathPlugins, | ||
| final Path configPath, | ||
| final boolean forbidPrivateIndexSettings, | ||
| final Map<Class<? extends Plugin>, Class<? extends Plugin>> extendedPlugins |
There was a problem hiding this comment.
Why do we need extendedPlugins here if we already have classpathPlugins ? If we need to use PluginInfo here, let's do that instead
There was a problem hiding this comment.
Because extendedPlugins contains the relationships between the classpath plugins.
i.e. classpathPluginA extends classpathPluginB.
There was a problem hiding this comment.
But this has to pass though PluginInfo::extendedPlugins, right? This is how the relationship between plugins is designed?
There was a problem hiding this comment.
It does in one of the constructors in this class. I chose to support multiple different constructors to continue to support use-cases such as this. For the majority of tests using classpathPlugins they don't need to declare extensions. For test cases that would need to declare extensions, they can use the respective constructor that supports it.
There was a problem hiding this comment.
I would suggest to have 2 constructors, as we did for Node:
- one that accepts
final Collection<Class<? extends Plugin>> classpathPlugins, - one that accepts
final Collection<PluginInfo> classpathPlugins,
There was a problem hiding this comment.
Got it, I will need some time to analyze the different usages of the 7 different constructors in this class. It looks like this class has evolved over time to support many different scenarios for testing.
|
❌ Gradle check result for 07df814: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
❌ Gradle check result for 49ec944: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
❕ Gradle check result for f9bc2ae: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
| @@ -63,4 +64,9 @@ public Path nodeConfigPath(int nodeOrdinal) { | |||
| public Collection<Class<? extends Plugin>> nodePlugins() { | |||
| return Collections.emptyList(); | |||
| } | |||
|
|
|||
| /** Returns a map corresponding to plugin dependencies to other classpath plugins */ | |||
| public Map<Class<? extends Plugin>, Class<? extends Plugin>> extendedPlugins() { | |||
There was a problem hiding this comment.
I think this is the last place where we need PluginInfo (from classpath): public Class<PluginInfo> classpathPlugins() {
Description
This PR enables testing for ExtensiblePlugins using the classpath plugin pattern present in
internalClusterTest.To do this, this PR tries to load all extensions for classpath plugins by defining an extendedPlugin relationship between a classpath plugin and all other classpath plugins. In normal scenarios, a plugin will declare that it extends another plugin through the
extendedPluginskeyword that it places in build.gradle (example). This PR also puts logic in place to ensure that any extensions are only loaded once.Additional Context
I ran into this issue while using the integration test framework in the security plugin to test out a change I have been experimenting with. The integration test framework utilizes classpath plugins and it was impossible to load an extensible plugin and its extension simultaneously and have the extensions loaded.
Check List
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.