Use instance of LockService instantiated in JobScheduler through Guice

[cwperks]

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.

[cwperks]

The CI will fail until companion JS PR is merged

[heemin32]

@cwperks Could you also update the change log?

[cwperks]

@cwperks Could you also update the change log?

Added

[heemin32]

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

[cwperks]

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

[heemin32]

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

[cwperks]

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

[cwperks]

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]

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

[cwperks]

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.

[cwperks]

Added the checks

[heemin32]

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

[cwperks]

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.

[heemin32]

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.

[cwperks]

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?

[heemin32]

You can call Ip2GeoLockService.getLockService()

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

[heemin32]

Can we move this class to its own file?

[cwperks]

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.

[heemin32]

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.

[cwperks]

Switched to package-private

[cwperks]

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)

[heemin32]

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

[cwperks]

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

[heemin32]

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

[heemin32]

LGTM. Thanks!