Use instance of LockService instantiated in JobScheduler through Guice #677

cwperks · 2024-08-27T15:08:00Z

Description

Companion JS PR: opensearch-project/job-scheduler#670

This PR shows how Geospatial can be refactored to use the instance of the LockService that is initialized in Job Scheduler's createComponents.

This PR is part of an effort to remove usages of ThreadContext.stashContext across the plugins: opensearch-project/opensearch-plugins#238

Currently, geospatial instantiates its own instance of the LockService by passing in the Client given to geospatial through createComponents.

As part of the effort to Strengthen System Indices in the Plugin Ecosystem, plugins will be restricted to only perform transport actions to their own system indices. This PR is to ensure that Geospatial uses the LockService instantiated by JS (which has permission to JS system indices) vs creating its own LockService.

Related Issues

Related to opensearch-project/security#4439

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Signed-off-by: Craig Perkins <[email protected]>

…heduler Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-08-27T15:09:19Z

The CI will fail until companion JS PR is merged

Signed-off-by: Craig Perkins <[email protected]>

heemin32 · 2025-01-21T18:00:59Z

@cwperks Could you also update the change log?

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2025-01-21T18:12:23Z

@cwperks Could you also update the change log?

Added

src/main/java/org/opensearch/geospatial/plugin/GeospatialPlugin.java

heemin32 · 2025-01-21T18:42:03Z

src/main/java/org/opensearch/geospatial/ip2geo/common/Ip2GeoLockService.java

@@ -43,6 +43,19 @@ public Ip2GeoLockService(final ClusterService clusterService, final Client clien
        this.lockService = new LockService(client, clusterService);


Shouldn't we block this constructor if we don't want plugin to instantiate their own lock service?

Yes, but first all usages in the default distribution need to be addressed: https://github.com/search?q=org%3Aopensearch-project%20LockService&type=code

I'll raise a PR in JS to formally mark the constructor as Deprecated

Oh. I mean, public Ip2GeoLockService(final ClusterService clusterService, final Client client) not public LockService(final ClusterService clusterService, final Client client)

yes, good point. The tests need to be updated in Ip2GeoLockServiceTests. Taking a look now.

Removed the constructor of Ip2GeoLockService that accepts a Client. There are still instances where LockService is instantiated in tests that I plan to remove in a subsequent PR. The difficulty I am facing now is creating a mock instance of LockService because its a final class in the JS SPI.

heemin32 · 2025-01-21T18:42:51Z

src/main/java/org/opensearch/geospatial/ip2geo/common/Ip2GeoLockService.java

+     * @param clusterService the cluster service
+     */
+    public Ip2GeoLockService(final ClusterService clusterService) {
+        this.clusterService = clusterService;


We might need to add lockService not initialized check in every Ip2GeoLockService methods.

It should never happen (i.e. onNodeStarted will always be called and the lock service will get set accordingly) but these checks would be good to put in none-the-less.

Added the checks

heemin32 · 2025-01-21T19:00:24Z

src/main/java/org/opensearch/geospatial/ip2geo/common/Ip2GeoLockService.java

@@ -30,7 +30,7 @@ public class Ip2GeoLockService {
    public static final long LOCK_DURATION_IN_SECONDS = 300l;
    public static final long RENEW_AFTER_IN_SECONDS = 120l;
    private final ClusterService clusterService;
-    private final LockService lockService;
+    private LockService lockService;


Wouldn't it be better to make Ip2GeoLockService to be LifecycleComponent and get injected both cluster service and lock service?

It is still being returned from createComponents so that its injectable. The difference in this PR is that it now implements onNodeStarted to get the instance of LockService instantiated by the job scheduler from the GuiceHolder and then sets it on the Ip2GeoLockService (Lock Service Wrapper) instance.

I was thinking like

public class Ip2GeoLockService implements LifecycleComponent { private final static Ip2GeoLockService lockService; public static Ip2GeoLockService getLockService() { return lockService; } @Inject public Ip2GeoLockService(final ClusterService clusterService, final LockService lockService) { } } @Override public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() { final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(2); services.add(Ip2GeoListener.class); services.add(Ip2GeoLockService.class); return services; }

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.

That looks like a good solution, but how do you refer to the instance of the LifecycleComponent that's created?

For instance this line:

DatasourceRunner.getJobRunnerInstance() .initialize(this.clusterService, this.datasourceUpdateService, this.ip2GeoExecutor, this.datasourceDao, this.ip2GeoLockService);

It takes the ip2GeoLockService as the last arg. When using the service class, how do you get a hold of an instance?

You can call Ip2GeoLockService.getLockService()

However, I think having a GuiceHolder class might be better not to expose that static method.

Signed-off-by: Craig Perkins <[email protected]>

heemin32 · 2025-01-21T19:28:51Z

src/main/java/org/opensearch/geospatial/plugin/GeospatialPlugin.java

+            .initialize(this.clusterService, this.datasourceUpdateService, this.ip2GeoExecutor, this.datasourceDao, this.ip2GeoLockService);
+    }
+
+    public static class GuiceHolder implements LifecycleComponent {


Can we move this class to its own file?

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.

It does not matter where and how the class is defined. As long as you return your class in getGuiceServiceClasses(), the dependency will get injected.

@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; }

Anyway, I think having this class inside Plugin class might make sense if we want it to be used only inside this Plugin class.
private or package private might be better instead of public then.

Switched to package-private

Looks like it needs to be public:

ref: https://github.com/opensearch-project/geospatial/actions/runs/12895082709/job/35955219215?pr=677

? java.lang.AssertionError: Wrong access modifiers on public org.opensearch.geospatial.plugin.GeospatialPlugin$GuiceHolder(org.opensearch.jobscheduler.spi.utils.LockService) ? at org.opensearch.common.inject.DefaultConstructionProxyFactory$1.newInstance(DefaultConstructionProxyFactory.java:69) ? at org.opensearch.common.inject.ConstructorInjector.construct(ConstructorInjector.java:101) ? at org.opensearch.common.inject.ConstructorBindingImpl$Factory.get(ConstructorBindingImpl.java:140) ? at org.opensearch.common.inject.InjectorImpl$4$1.call(InjectorImpl.java:753) ? at org.opensearch.common.inject.InjectorImpl.callInContext(InjectorImpl.java:809) ? at org.opensearch.common.inject.InjectorImpl$4.get(InjectorImpl.java:748) ? at org.opensearch.common.inject.InjectorImpl.getInstance(InjectorImpl.java:[792](https://github.com/opensearch-project/geospatial/actions/runs/12895082709/job/35955219215?pr=677#step:6:793)) ? at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) ? at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708) ? at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) ? at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ? at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921) ? at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ? at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682) ? at org.opensearch.node.Node.<init>(Node.java:1548) ? at org.opensearch.node.Node.<init>(Node.java:452)

In such case, making methods as package private might be enough.

Any of the overridden methods need to be marked public. I marked getLockService as package-private.

Yes. I was talking only for those static methods :) Sorry for the confusion.

src/main/java/org/opensearch/geospatial/plugin/GeospatialPlugin.java

Signed-off-by: Craig Perkins <[email protected]>

heemin32

LGTM. Thanks!

#677) * WIP to show Geospatial plugin using LockService instance from JS Signed-off-by: Craig Perkins <[email protected]> * Use instance of LockService from Guice that is instantiated by Job Scheduler Signed-off-by: Craig Perkins <[email protected]> * Fix failing tests Signed-off-by: Craig Perkins <[email protected]> * Add to CHANGELOG Signed-off-by: Craig Perkins <[email protected]> * Address comments Signed-off-by: Craig Perkins <[email protected]> * Add checks to see if initialized Signed-off-by: Craig Perkins <[email protected]> * Remove constructor that accepts client Signed-off-by: Craig Perkins <[email protected]> * Switch to package-private Signed-off-by: Craig Perkins <[email protected]> * package-private Signed-off-by: Craig Perkins <[email protected]> * public Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 847be33)

#677) (#713) * WIP to show Geospatial plugin using LockService instance from JS Signed-off-by: Craig Perkins <[email protected]> * Use instance of LockService from Guice that is instantiated by Job Scheduler Signed-off-by: Craig Perkins <[email protected]> * Fix failing tests Signed-off-by: Craig Perkins <[email protected]> * Add to CHANGELOG Signed-off-by: Craig Perkins <[email protected]> * Address comments Signed-off-by: Craig Perkins <[email protected]> * Add checks to see if initialized Signed-off-by: Craig Perkins <[email protected]> * Remove constructor that accepts client Signed-off-by: Craig Perkins <[email protected]> * Switch to package-private Signed-off-by: Craig Perkins <[email protected]> * package-private Signed-off-by: Craig Perkins <[email protected]> * public Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 847be33) Co-authored-by: Craig Perkins <[email protected]>

cwperks added 2 commits August 27, 2024 10:46

WIP to show Geospatial plugin using LockService instance from JS

6095e0c

Signed-off-by: Craig Perkins <[email protected]>

Use instance of LockService from Guice that is instantiated by Job Sc…

6a5d01b

…heduler Signed-off-by: Craig Perkins <[email protected]>

cwperks requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei and martin-gaievski as code owners August 27, 2024 15:08

cwperks mentioned this pull request Aug 27, 2024

Return instance of LockService from createComponents opensearch-project/job-scheduler#670

Merged

5 tasks

cwperks added 2 commits September 5, 2024 08:34

Fix failing tests

bc6544b

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into lock-service-guice

35c5807

heemin32 added the backport 2.x Backport PR to 2.x branch label Jan 21, 2025

Add to CHANGELOG

3de07ff

Signed-off-by: Craig Perkins <[email protected]>

heemin32 reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/geospatial/plugin/GeospatialPlugin.java Outdated Show resolved Hide resolved

heemin32 reviewed Jan 21, 2025

View reviewed changes

cwperks added 2 commits January 21, 2025 14:00

Address comments

b5d63f2

Signed-off-by: Craig Perkins <[email protected]>

Add checks to see if initialized

0a0377f

Signed-off-by: Craig Perkins <[email protected]>

heemin32 reviewed Jan 21, 2025

View reviewed changes

src/main/java/org/opensearch/geospatial/plugin/GeospatialPlugin.java Outdated Show resolved Hide resolved

cwperks added 4 commits January 21, 2025 14:53

Remove constructor that accepts client

dfcc031

Signed-off-by: Craig Perkins <[email protected]>

Switch to package-private

d7fd6c5

Signed-off-by: Craig Perkins <[email protected]>

package-private

6c1d855

Signed-off-by: Craig Perkins <[email protected]>

public

83677cd

Signed-off-by: Craig Perkins <[email protected]>

heemin32 approved these changes Jan 21, 2025

View reviewed changes

junqiu-lei approved these changes Jan 21, 2025

View reviewed changes

heemin32 merged commit 847be33 into opensearch-project:main Jan 21, 2025
9 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 21, 2025

[Backport 2.x] Use instance of LockService instantiated in JobScheduler through Guice #713

Merged

cwperks mentioned this pull request Jan 31, 2025

Replace usages of ThreadContext.stashContext with pluginSubject.runAs #715

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use instance of LockService instantiated in JobScheduler through Guice #677

Use instance of LockService instantiated in JobScheduler through Guice #677

cwperks commented Aug 27, 2024

cwperks commented Aug 27, 2024

heemin32 commented Jan 21, 2025

cwperks commented Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025 •

edited

Loading

heemin32 Jan 21, 2025 •

edited

Loading

cwperks Jan 21, 2025 •

edited

Loading

heemin32 Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025 •

edited

Loading

cwperks Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025

cwperks Jan 21, 2025

heemin32 Jan 21, 2025 •

edited

Loading

heemin32 left a comment

		@@ -43,6 +43,19 @@ public Ip2GeoLockService(final ClusterService clusterService, final Client clien
		this.lockService = new LockService(client, clusterService);

Use instance of LockService instantiated in JobScheduler through Guice #677

Use instance of LockService instantiated in JobScheduler through Guice #677

Conversation

cwperks commented Aug 27, 2024

Description

Related Issues

Check List

cwperks commented Aug 27, 2024

heemin32 commented Jan 21, 2025

cwperks commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

heemin32 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

cwperks Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

heemin32 left a comment

Choose a reason for hiding this comment

cwperks Jan 21, 2025 •

edited

Loading

heemin32 Jan 21, 2025 •

edited

Loading

cwperks Jan 21, 2025 •

edited

Loading

heemin32 Jan 21, 2025 •

edited

Loading

heemin32 Jan 21, 2025 •

edited

Loading