Fix k8sAndWorker mode in a zookeeper-less environment #15445
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...ord-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesOverlordModuleTest.java
Fixed
Show fixed
Hide fixed
...ord-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesOverlordModuleTest.java
Fixed
Show fixed
Hide fixed
| when(runnerConfig.getWorkerType()).thenReturn(RemoteTaskRunnerFactory.TYPE_NAME); | ||
| when(parentProvider.get()).thenReturn(remoteTaskRunnerFactory); | ||
| assertSame(remoteTaskRunnerFactory, module.provideRemoteTaskRunnerFactory(parentProvider, runnerConfig)); | ||
| } |
There was a problem hiding this comment.
Can we add a test here to see that the KubernetesAndWorkerTaskRunnerFactory is initialized correctly please
There was a problem hiding this comment.
I updated the code, this Module change is reverted.
| HttpRemoteTaskRunnerFactory httpRemoteTaskRunnerFactory, | ||
| RemoteTaskRunnerFactory remoteTaskRunnerFactory, | ||
| @Nullable RemoteTaskRunnerFactory remoteTaskRunnerFactory, |
There was a problem hiding this comment.
The pattern I've usually seen here is to inject a Provider<X> into the constructor and then use the config to decide whether or not to call Provider.get() https://github.com/google/guice/wiki/InjectingProviders#injecting-providers.
Would this technique work? It seems slightly easier to follow as RemoteTaskRunnerFactory is already injectable from another module, so I'm not certain what the expected behavior is when 2 modules try to inject the same class.
There was a problem hiding this comment.
Agree, the provider pattern is simpler. Updated the code.
There was a problem hiding this comment.
I think the code changes look good.
To prove this works, a module test would be helpful here to validate that the KubernetesAndWorkerTaskRunnerFactory can be injected without any dependencies for the remoteTaskRunnerFactory being provided.
After the test, +1 from me
| @@ -33,7 +34,11 @@ public class KubernetesAndWorkerTaskRunnerFactory implements TaskRunnerFactory<K | |||
|
|
|||
| private final KubernetesTaskRunnerFactory kubernetesTaskRunnerFactory; | |||
| private final HttpRemoteTaskRunnerFactory httpRemoteTaskRunnerFactory; | |||
There was a problem hiding this comment.
Should this also be injected as a Provider since it is not needed when the remote task runner factory is configured?
There was a problem hiding this comment.
Changed HttpRemoteTaskRunnerFactory to be injected as Provider.
I find it's very hard to do module test on the Guice injection behavior, especially on the Provider pattern. The bindings appear to behave differently during testing compared to when the actual service starts. |
Added module test to verify the binding works |
Description
Fix k8sAndWorker mode in a zookeeper-less environment
Release note
Fix this issue #15431
Key changed/added classes in this PR
KubernetesAndWorkerTaskRunnerFactorychange to bindTaskRunnerFactoryinstanceKubernetesOverlordModulechange to provideTaskRunnerFactorybindingThis PR has: