You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "georgew5656 (via GitHub)" <gi...@apache.org> on 2024/09/27 17:26:49 UTC
[PR] Refactor pod template logic to make it easier to test (druid)
georgew5656 opened a new pull request, #17178:
URL: https://github.com/apache/druid/pull/17178
This PR is a refactoring of some of the classes around TaskAdapter in the kubernetes-overlord-extensions. There shouldn't really be any logic changes.
### Description
- Instead of constructing the TaskAdapter instance in KubernetesTaskRunnerFactory, include a TaskAdapter provider in KubernetesOverlordModule that gets injected into KubernetesTaskRunnerFactory. This makes the adapter logic easier to test since we can just look at the injected object in KubernetesOverlordModule tests rather than trying to look at the KubernetesTaskRunner object that KubernetesTaskRunnerFactory creates.
- Move the pod template logic in PodTemplateAdapter into a new PodTemplateSelector class that exposes getPodTemplateForTask(task). This makes it easier to test because we can test pod template selection logic independently from the logic in PodTemplateAdapter (e.g. the logic to add specific env variables to the generated pod template can be tested independently of the pod templates themselves since we can just pass in a TestPodTemplateSelector).
#### Release note
-Refactor classes in kubernetes-overlord-extensions
##### Key changed/added classes in this PR
* `KubernetesOverlordModule`
* `KubernetesTaskRunnerFactory`
* `PodTemplateTaskAdapter`
* `PodTemplateSelector`
<hr>
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor pod template logic to make it easier to test (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17178:
URL: https://github.com/apache/druid/pull/17178#discussion_r1778965934
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesOverlordModule.java:
##########
@@ -162,6 +176,70 @@
: injector.getInstance(RemoteTaskRunnerFactory.class);
}
+ /**
+ * Provides a TaskAdapter instance for the KubernetesTaskRunner.
+ */
+ @Provides
+ @LazySingleton
+ TaskAdapter provideTaskAdapter(
+ DruidKubernetesClient client,
+ Properties properties,
+ KubernetesTaskRunnerConfig kubernetesTaskRunnerConfig,
+ TaskConfig taskConfig,
+ StartupLoggingConfig startupLoggingConfig,
+ @Self DruidNode druidNode,
+ @Smile ObjectMapper smileMapper,
+ TaskLogs taskLogs,
+ Supplier<KubernetesTaskRunnerDynamicConfig> dynamicConfigRef
+ )
+ {
+ String adapter = properties.getProperty(String.format(
+ Locale.ROOT,
+ "%s.%s.adapter.type",
+ IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX,
+ "k8s"
+ ));
+
+ if (adapter != null && !MultiContainerTaskAdapter.TYPE.equals(adapter) && kubernetesTaskRunnerConfig.isSidecarSupport()) {
Review Comment:
## Deprecated method or constructor invocation
Invoking [KubernetesTaskRunnerConfig.isSidecarSupport](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8359)
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesOverlordModule.java:
##########
@@ -162,6 +176,70 @@
: injector.getInstance(RemoteTaskRunnerFactory.class);
}
+ /**
+ * Provides a TaskAdapter instance for the KubernetesTaskRunner.
+ */
+ @Provides
+ @LazySingleton
+ TaskAdapter provideTaskAdapter(
+ DruidKubernetesClient client,
+ Properties properties,
+ KubernetesTaskRunnerConfig kubernetesTaskRunnerConfig,
+ TaskConfig taskConfig,
+ StartupLoggingConfig startupLoggingConfig,
+ @Self DruidNode druidNode,
+ @Smile ObjectMapper smileMapper,
+ TaskLogs taskLogs,
+ Supplier<KubernetesTaskRunnerDynamicConfig> dynamicConfigRef
+ )
+ {
+ String adapter = properties.getProperty(String.format(
+ Locale.ROOT,
+ "%s.%s.adapter.type",
+ IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX,
+ "k8s"
+ ));
+
+ if (adapter != null && !MultiContainerTaskAdapter.TYPE.equals(adapter) && kubernetesTaskRunnerConfig.isSidecarSupport()) {
+ throw new IAE(
+ "Invalid pod adapter [%s], only pod adapter [%s] can be specified when sidecarSupport is enabled",
+ adapter,
+ MultiContainerTaskAdapter.TYPE
+ );
+ }
+
+ if (MultiContainerTaskAdapter.TYPE.equals(adapter) || kubernetesTaskRunnerConfig.isSidecarSupport()) {
Review Comment:
## Deprecated method or constructor invocation
Invoking [KubernetesTaskRunnerConfig.isSidecarSupport](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8360)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org