Use instance of LockService instantiated in JobScheduler through Guice #677
+126
−26
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,14 @@ | |
| import org.opensearch.action.ActionRequest; | ||
| import org.opensearch.client.Client; | ||
| import org.opensearch.cluster.metadata.IndexNameExpressionResolver; | ||
| import org.opensearch.cluster.node.DiscoveryNode; | ||
| import org.opensearch.cluster.node.DiscoveryNodes; | ||
| import org.opensearch.cluster.service.ClusterService; | ||
| import org.opensearch.common.collect.MapBuilder; | ||
| import org.opensearch.common.inject.Inject; | ||
| import org.opensearch.common.lifecycle.Lifecycle; | ||
| import org.opensearch.common.lifecycle.LifecycleComponent; | ||
| import org.opensearch.common.lifecycle.LifecycleListener; | ||
| import org.opensearch.common.settings.ClusterSettings; | ||
| import org.opensearch.common.settings.IndexScopedSettings; | ||
| import org.opensearch.common.settings.Setting; | ||
|
|
@@ -75,7 +79,9 @@ | |
| import org.opensearch.index.mapper.Mapper; | ||
| import org.opensearch.indices.SystemIndexDescriptor; | ||
| import org.opensearch.ingest.Processor; | ||
| import org.opensearch.jobscheduler.spi.utils.LockService; | ||
| import org.opensearch.plugins.ActionPlugin; | ||
| import org.opensearch.plugins.ClusterPlugin; | ||
| import org.opensearch.plugins.IngestPlugin; | ||
| import org.opensearch.plugins.MapperPlugin; | ||
| import org.opensearch.plugins.Plugin; | ||
|
|
@@ -96,11 +102,22 @@ | |
| * to interact with Cluster. | ||
| */ | ||
| @Log4j2 | ||
| public class GeospatialPlugin extends Plugin | ||
| implements | ||
| IngestPlugin, | ||
| ActionPlugin, | ||
| MapperPlugin, | ||
| SearchPlugin, | ||
| SystemIndexPlugin, | ||
| ClusterPlugin { | ||
| private Ip2GeoCachedDao ip2GeoCachedDao; | ||
| private DatasourceDao datasourceDao; | ||
| private GeoIpDataDao geoIpDataDao; | ||
| private URLDenyListChecker urlDenyListChecker; | ||
| private ClusterService clusterService; | ||
| private Ip2GeoLockService ip2GeoLockService; | ||
| private Ip2GeoExecutor ip2GeoExecutor; | ||
| private DatasourceUpdateService datasourceUpdateService; | ||
|
|
||
| @Override | ||
| public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) { | ||
|
|
@@ -129,7 +146,10 @@ public void onIndexModule(IndexModule indexModule) { | |
|
|
||
| @Override | ||
| public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() { | ||
| final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(2); | ||
| services.add(Ip2GeoListener.class); | ||
| services.add(GuiceHolder.class); | ||
| return services; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -158,20 +178,10 @@ public Collection<Object> createComponents( | |
| IndexNameExpressionResolver indexNameExpressionResolver, | ||
| Supplier<RepositoriesService> repositoriesServiceSupplier | ||
| ) { | ||
| this.clusterService = clusterService; | ||
| this.datasourceUpdateService = new DatasourceUpdateService(clusterService, datasourceDao, geoIpDataDao, urlDenyListChecker); | ||
| this.ip2GeoExecutor = new Ip2GeoExecutor(threadPool); | ||
| this.ip2GeoLockService = new Ip2GeoLockService(clusterService); | ||
|
|
||
| return List.of( | ||
| UploadStats.getInstance(), | ||
|
|
@@ -265,4 +275,48 @@ public List<AggregationSpec> getAggregations() { | |
|
|
||
| return List.of(geoHexGridSpec); | ||
| } | ||
|
|
||
| @Override | ||
| public void onNodeStarted(DiscoveryNode localNode) { | ||
| LockService lockService = GuiceHolder.getLockService(); | ||
| ip2GeoLockService.initialize(lockService); | ||
|
|
||
| DatasourceRunner.getJobRunnerInstance() | ||
| .initialize(this.clusterService, this.datasourceUpdateService, this.ip2GeoExecutor, this.datasourceDao, this.ip2GeoLockService); | ||
| } | ||
|
|
||
| public static class GuiceHolder implements LifecycleComponent { | ||
|
There was a problem hiding this comment. Can we move this class to its own file? There was a problem hiding this comment. Its a static inner class so I think it needs to be defined in this file. This is following a similar pattern present in the security plugin. This is a way to do dependency injection outside of the transport layer. There was a problem hiding this comment. It does not matter where and how the class is defined. As long as you return your class in Anyway, I think having this class inside Plugin class might make sense if we want it to be used only inside this Plugin class. There was a problem hiding this comment. Switched to package-private There was a problem hiding this comment. Looks like it needs to be public: There was a problem hiding this comment. In such case, making methods as package private might be enough. There was a problem hiding this comment. Any of the overridden methods need to be marked public. I marked There was a problem hiding this comment. Yes. I was talking only for those static methods :) Sorry for the confusion. |
||
|
|
||
| private static LockService lockService; | ||
|
|
||
| @Inject | ||
| public GuiceHolder(final LockService lockService) { | ||
| GuiceHolder.lockService = lockService; | ||
| } | ||
|
|
||
| static LockService getLockService() { | ||
| return lockService; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() {} | ||
|
|
||
| @Override | ||
| public Lifecycle.State lifecycleState() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void addLifecycleListener(LifecycleListener listener) {} | ||
|
|
||
| @Override | ||
| public void removeLifecycleListener(LifecycleListener listener) {} | ||
|
|
||
| @Override | ||
| public void start() {} | ||
|
|
||
| @Override | ||
| public void stop() {} | ||
|
|
||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.geospatial; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
|
|
||
| import org.opensearch.common.lifecycle.LifecycleComponent; | ||
| import org.opensearch.geospatial.ip2geo.listener.Ip2GeoListener; | ||
| import org.opensearch.geospatial.plugin.GeospatialPlugin; | ||
|
|
||
| /** | ||
| * This class is needed for ClusterSettingsHelper.createMockNode to instantiate a test instance of the | ||
| * GeospatialPlugin without the JobSchedulerPlugin installed. Without overriding this class, the | ||
| * GeospatialPlugin would try to Inject JobScheduler's LockService in the GuiceHolder which will | ||
| * fail because JobScheduler is not installed | ||
| */ | ||
| public class TestGeospatialPlugin extends GeospatialPlugin { | ||
| @Override | ||
| public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() { | ||
| final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(1); | ||
| services.add(Ip2GeoListener.class); | ||
| return services; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Wouldn't it be better to make Ip2GeoLockService to be LifecycleComponent and get injected both cluster service and lock service?
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.
It is still being returned from
createComponentsso that its injectable. The difference in this PR is that it now implementsonNodeStartedto get the instance of LockService instantiated by the job scheduler from the GuiceHolder and then sets it on the Ip2GeoLockService (Lock Service Wrapper) instance.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 was thinking like
This way, we don't need an additional class.
However, this approach opens up the possibility for the
public static Ip2GeoLockService getLockService()method to be used across different parts of the code which is not desirable. Therefore, I think having a separate class might be better in that regard.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.
That looks like a good solution, but how do you refer to the instance of the LifecycleComponent that's created?
For instance this line:
It takes the
ip2GeoLockServiceas the last arg. When using the service class, how do you get a hold of an instance?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.
You can call
Ip2GeoLockService.getLockService()However, I think having a
GuiceHolderclass might be better not to expose that static method.