kfaraz commented on code in PR #18105:
URL: https://github.com/apache/druid/pull/18105#discussion_r2136853055
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java:
##########
@@ -80,12 +82,22 @@ public Pod launchPeonJobAndWaitForStart(Job job, Task task,
long howLong, TimeUn
long start = System.currentTimeMillis();
// launch job
return clientApi.executeRequest(client -> {
- client.batch().v1().jobs().inNamespace(namespace).resource(job).create();
String jobName = job.getMetadata().getName();
- log.info("Successfully submitted job: %s ... waiting for job to launch",
jobName);
+
+ log.info("Submitting job [%s] for task [%s]",
job.getMetadata().getName(), task.getId());
+ client.batch()
+ .v1()
+ .jobs()
+ .inNamespace(namespace)
+ .resource(job)
+ .create();
+ log.info("Submitted job [%s] for task [%s]. Waiting for POD to launch",
jobName, task.getId());
Review Comment:
```suggestion
log.info("Submitted job[%s] for task[%s]. Waiting for POD to launch.",
jobName, task.getId());
```
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java:
##########
@@ -80,12 +82,22 @@ public Pod launchPeonJobAndWaitForStart(Job job, Task task,
long howLong, TimeUn
long start = System.currentTimeMillis();
// launch job
return clientApi.executeRequest(client -> {
- client.batch().v1().jobs().inNamespace(namespace).resource(job).create();
String jobName = job.getMetadata().getName();
- log.info("Successfully submitted job: %s ... waiting for job to launch",
jobName);
+
+ log.info("Submitting job [%s] for task [%s]",
job.getMetadata().getName(), task.getId());
Review Comment:
```suggestion
log.info("Submitting job[%s] for task[%s].",
job.getMetadata().getName(), task.getId());
```
##########
docs/development/extensions-core/k8s-jobs.md:
##########
@@ -772,28 +772,28 @@ Ensure that when you are running task pods in another
namespace, your task pods
Should you require the needed permissions for interacting across Kubernetes
namespaces, you can specify a kubeconfig file, and provided the necessary
permissions. You can then use the `KUBECONFIG` environment variable to allow
your Overlord deployment to find your kubeconfig file. Refer to the [Kubernetes
documentation](https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/)
for more information.
### Properties
-| Property | Possible Values | Description | Default | Required |
-| --- | --- | --- | --- | --- |
-| `druid.indexer.runner.namespace` | `String` | If Overlord and task pods are
running in different namespaces, specify the Overlord namespace. | - | Yes |
-| `druid.indexer.runner.overlordNamespace` | `String` | Only applicable when
using Custom Template Pod Adapter. If Overlord and task pods are running in
different namespaces, specify the Overlord namespace. <br /> Warning: You need
to stop all running tasks in Druid to change this property. Failure to do so
will lead to duplicate data and metadata inconsistencies. | `""` | No |
-| `druid.indexer.runner.k8sTaskPodNamePrefix` | `String` | Use this if you
want to change your task name to contain a more human-readable prefix. Maximum
30 characters. Special characters `: - . _` will be ignored. <br /> Warning:
You need to stop all running tasks in Druid to change this property. Failure to
do so will lead to duplicate data and metadata inconsistencies. | `""` | No |
-| `druid.indexer.runner.debugJobs` | `boolean` | Clean up K8s jobs after tasks
complete. | False | No |
-| `druid.indexer.runner.sidecarSupport` | `boolean` | Deprecated, specify
adapter type as runtime property `druid.indexer.runner.k8s.adapter.type:
overlordMultiContainer` instead. If your overlord pod has sidecars, this will
attempt to start the task with the same sidecars as the overlord pod. | False |
No |
-| `druid.indexer.runner.primaryContainerName` | `String` | If running with
sidecars, the `primaryContainerName` should be that of your druid container
like `druid-overlord`. | First container in `podSpec` list | No |
-| `druid.indexer.runner.kubexitImage` | `String` | Used kubexit project to
help shutdown sidecars when the main pod completes. Otherwise, jobs with
sidecars never terminate. | karlkfi/kubexit:latest | No |
-| `druid.indexer.runner.disableClientProxy` | `boolean` | Use this if you have
a global http(s) proxy and you wish to bypass it. | false | No |
-| `druid.indexer.runner.maxTaskDuration` | `Duration` | Max time a task is
allowed to run for before getting killed. | `PT4H` | No |
-| `druid.indexer.runner.taskCleanupDelay` | `Duration` | How long do jobs stay
around before getting reaped from K8s. | `P2D` | No |
-| `druid.indexer.runner.taskCleanupInterval` | `Duration` | How often to check
for jobs to be reaped. | `PT10M` | No |
-| `druid.indexer.runner.taskJoinTimeout` | `Duration` | Timeout for gathering
metadata about existing tasks on startup. | `PT1M` | No |
-| `druid.indexer.runner.K8sjobLaunchTimeout` | `Duration` | How long to wait
to launch a K8s task before marking it as failed, on a resource constrained
cluster it may take some time. | `PT1H` | No |
-| `druid.indexer.runner.javaOptsArray` | `JsonArray` | java opts for the task.
| `-Xmx1g` | No |
-| `druid.indexer.runner.labels` | `JsonObject` | Additional labels you want to
add to peon pod. | `{}` | No |
-| `druid.indexer.runner.annotations` | `JsonObject` | Additional annotations
you want to add to peon pod. | `{}` | No |
-| `druid.indexer.runner.peonMonitors` | `JsonArray` | Overrides
`druid.monitoring.monitors`. Use this property if you don't want to inherit
monitors from the Overlord. | `[]` | No |
+| Property | Possible Values |
Description | Default | Required |
Review Comment:
The formatting followed in Druid docs is to avoid unnecessary spaces.
So a table typically looks like this:
```
|Column 1|Column 2|Column 3|
|--------|--------|--------|
|Value 11|Value 12|Long value 13|
|This is a much longer value 21|v22|v23|
```
I would advise either reverting the formatting changes here (to keep this
patch focused on the code change) or sticking to the above format.
I think in the long term, we should also explore the idea of standard
formatting for tables etc in docs. I don't think there is any checkstyle or
some kind of lint checker that is currently run as a part of docs CI.
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java:
##########
@@ -94,7 +106,7 @@ public Pod launchPeonJobAndWaitForStart(Job job, Task task,
long howLong, TimeUn
}, howLong, timeUnit);
if (result == null) {
- throw new IllegalStateException("K8s pod for the task [%s] appeared
and disappeared. It can happen if the task was canceled");
+ throw new ISE(StringUtils.format("K8s pod for the task [%s] appeared
and disappeared. It can happen if the task was canceled", task.getId()));
Review Comment:
I think `ISE` already takes a format string with args, we need not
explicitly use `StringUtils.format`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
