gianm commented on code in PR #17446:
URL: https://github.com/apache/druid/pull/17446#discussion_r1831582139
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -256,13 +268,18 @@ protected TaskLocation getTaskLocation()
if (taskLocation == null) {
Optional<Pod> maybePod =
kubernetesClient.getPeonPod(taskId.getK8sJobName());
if (!maybePod.isPresent()) {
+ /* Arguably we should throw a exception here but leaving it as a warn
log to prevent unexpected errors.
+ If there is strange behavior during overlord restarts the operator
should look for this warn log.
+ */
+ log.warn("Could not get task location from k8s.");
return TaskLocation.unknown();
}
Pod pod = maybePod.get();
PodStatus podStatus = pod.getStatus();
if (podStatus == null || podStatus.getPodIP() == null) {
+ log.warn("Could not get task location from k8s.");
Review Comment:
Log the task id please.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesWorkItem.java:
##########
@@ -119,4 +122,9 @@ public Task getTask()
{
return task;
}
+
+ public SettableFuture<Boolean> getTaskActiveStatusFuture()
Review Comment:
Javadoc please about what this represents.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -256,13 +268,18 @@ protected TaskLocation getTaskLocation()
if (taskLocation == null) {
Optional<Pod> maybePod =
kubernetesClient.getPeonPod(taskId.getK8sJobName());
if (!maybePod.isPresent()) {
+ /* Arguably we should throw a exception here but leaving it as a warn
log to prevent unexpected errors.
+ If there is strange behavior during overlord restarts the operator
should look for this warn log.
+ */
+ log.warn("Could not get task location from k8s.");
Review Comment:
Log the task id please.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -174,11 +176,13 @@ private void writeTaskPayload(Task task) throws
IOException
* @return
* @throws IllegalStateException
*/
- protected synchronized TaskStatus join(long timeout) throws
IllegalStateException
+ protected synchronized TaskStatus join(long timeout, SettableFuture<Boolean>
taskActiveStatusFuture) throws IllegalStateException
Review Comment:
This is an odd pattern; it's more typical for a method to return
`ListenableFuture<T>`. If the caller needs to chain that onto a
`SettableFuture<T>`, the caller should attach a callback that sets the settable
future.
Please also add javadoc explaining what the returned future means.
--
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]
