Exit properly when the k8s cluster is not available. #256
Conversation
|
cc @ash211 |
| .pods() | ||
| .withName(kubernetesAppId) | ||
| .watch(loggingWatch)) { _ => | ||
| loggingWatch.start |
There was a problem hiding this comment.
nit: methods with side effects that have no parameters usually get the parentheses: .start()
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-http</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
I'm not fully following here -- why does this change to the logging watcher require adding explicit dependencies on jetty ? Or are these separate changes?
There was a problem hiding this comment.
It's required to only build the kubernetes sub module (much faster compare to a full build), e.g. :
build/mvn -Pkubernetes -Phadoop-2.6 -pl resource-managers/kubernetes/core installSince jars of jetty artifacts are shaded in the top-level pom.xml, we need to reference them explicitly here, like what we already did for guava libs.
There was a problem hiding this comment.
So if we didn't have this addition, would we still be able to build the whole module? I don't see any new jetty-related code in this diff, so it seems like the pom change is independent from the exiting on apiserver unavailable change.
cc @mccheah
There was a problem hiding this comment.
OK, I've removed this part to make this PR atomic. PTAL.
|
|
||
| def start(): Unit = { | ||
| if (interval > 0) { | ||
| scheduler.scheduleWithFixedDelay(logRunnable, 0, interval, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
I think a more direct fix for this LoggingPodStatusWatcher holding the JVM open issue is to make sure that the scheduleThreadPool is launching daemon threads vs non-daemon threads.
That way we don't need to introduce this additional start method and can leave the scheduler start as a side effect of the constructor (and other scenarios where the scheduler fails to get stopped won't hold the JVM open)
What if scheduler = ThreadUtils.newDaemonSingleThreadExecutor("logging-pod-status-watcher") ?
There was a problem hiding this comment.
+1 for using daemon threads, but IMHO it's better to explicitly start the timer so it's a bit easier to reason about the control flow.
|
@ash211 Updated, PTAL |
|
@lins05 it looks like the latest push doesn't build on Jenkins: |
|
@ash211 Fixed. Was in a hurry yesterday and forgot to run a full compilation. |
* Exit properly when the k8s cluster is not available. * add jetty to k8s module dependency so we can use only rebuild the k8s module. * CR * Fixed single thread scheduler. * Fixed scalastyle check. * CR
…-k8s#256) * Exit properly when the k8s cluster is not available. * add jetty to k8s module dependency so we can use only rebuild the k8s module. * CR * Fixed single thread scheduler. * Fixed scalastyle check. * CR
2 participants
Close #225