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

Fix test memory leak #88362

Merged
merged 6 commits into from
Jul 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions server/src/main/java/org/elasticsearch/plugins/PluginsService.java
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ public <T> List<? extends T> loadServiceProviders(Class<T> service) {
List<T> result = new ArrayList<>();

for (LoadedPlugin pluginTuple : plugins()) {
ServiceLoader.load(service, pluginTuple.loader()).iterator().forEachRemaining(c -> result.add(c));
result.addAll(createExtensions(service, pluginTuple.instance));
}

return Collections.unmodifiableList(result);
Expand Down Expand Up @@ -348,11 +348,18 @@ static <T> T createExtension(Class<? extends T> extensionClass, Class<T> extensi
throw new IllegalStateException("no public " + extensionConstructorMessage(extensionClass, extensionPointType));
}

if (constructors.length > 1) {
Constructor<T> constructor = constructors[0];
// Using modules and SPI requires that we declare the default no-arg constructor apart from our custom
// one arg constructor with a plugin.
if (constructors.length == 2) {
// we prefer the one arg constructor in this case
if (constructors[1].getParameterCount() > 0) {
constructor = constructors[1];
}
} else if (constructors.length > 1) {
throw new IllegalStateException("no unique public " + extensionConstructorMessage(extensionClass, extensionPointType));
}

final Constructor<T> constructor = constructors[0];
if (constructor.getParameterCount() > 1) {
throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,10 @@ public TestExtension() {}
public TestExtension(TestPlugin plugin) {

}

public TestExtension(TestPlugin plugin, String anotherArg) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added a constructor to make the test check that we disallow a service provider with more than 2 constructors. Previously this class was testing for existence of more than 1.


}
}
IllegalStateException e = expectThrows(
IllegalStateException.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,24 @@
import org.elasticsearch.immutablestate.ImmutableClusterStateHandler;
import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider;

import java.util.Arrays;
import java.util.Collection;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
* ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface
*/
public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider {
private static final Set<ImmutableClusterStateHandler<?>> handlers = ConcurrentHashMap.newKeySet();
private final IndexLifecycle plugin;

@Override
public Collection<ImmutableClusterStateHandler<?>> handlers() {
return handlers;
public ILMImmutableStateHandlerProvider() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Declaring this constructor to make sure module-info compiles properly, since SPI requires that we use a no-arg constructor. If we switch to using the META-INF/services file, this problem goes away, however our build 'validateModule' task fails, requiring that we also expose this service provider in our module provides section.

throw new IllegalStateException("Provider must be constructed using PluginsService");
}

public ILMImmutableStateHandlerProvider(IndexLifecycle plugin) {
this.plugin = plugin;
}

public static void registerHandlers(ImmutableClusterStateHandler<?>... stateHandlers) {
handlers.addAll(Arrays.asList(stateHandlers));
@Override
public Collection<ImmutableClusterStateHandler<?>> handlers() {
return plugin.immutableClusterStateHandlers();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.health.HealthIndicatorService;
import org.elasticsearch.immutablestate.ImmutableClusterStateHandler;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.license.XPackLicenseState;
Expand Down Expand Up @@ -159,6 +160,7 @@ public class IndexLifecycle extends Plugin implements ActionPlugin, HealthPlugin
private final SetOnce<SnapshotHistoryStore> snapshotHistoryStore = new SetOnce<>();
private final SetOnce<IlmHealthIndicatorService> ilmHealthIndicatorService = new SetOnce<>();
private final SetOnce<SlmHealthIndicatorService> slmHealthIndicatorService = new SetOnce<>();
private final SetOnce<ImmutableLifecycleAction> immutableLifecycleAction = new SetOnce<>();
private final Settings settings;

public IndexLifecycle(Settings settings) {
Expand Down Expand Up @@ -268,10 +270,8 @@ public Collection<Object> createComponents(
components.addAll(Arrays.asList(snapshotLifecycleService.get(), snapshotHistoryStore.get(), snapshotRetentionService.get()));
ilmHealthIndicatorService.set(new IlmHealthIndicatorService(clusterService));
slmHealthIndicatorService.set(new SlmHealthIndicatorService(clusterService));
immutableLifecycleAction.set(new ImmutableLifecycleAction(xContentRegistry, client, XPackPlugin.getSharedLicenseState()));

ILMImmutableStateHandlerProvider.registerHandlers(
new ImmutableLifecycleAction(xContentRegistry, client, XPackPlugin.getSharedLicenseState())
);
return components;
}

Expand Down Expand Up @@ -422,6 +422,10 @@ public List<RestHandler> getRestHandlers(
return actions;
}

List<ImmutableClusterStateHandler<?>> immutableClusterStateHandlers() {
return List.of(immutableLifecycleAction.get());
}

@Override
public Collection<HealthIndicatorService> getHealthIndicatorServices() {
return List.of(ilmHealthIndicatorService.get(), slmHealthIndicatorService.get());
Expand Down