Reap previously launched logviewer tasks #240
Conversation
Reap previously launched logviewer tasks if `mesos.logviewer.sidecar.enabled` is now `false`
|
The only visible side effect of this change is that you'll now see this line every 5 minutes if the Do you think this is a problem? I'm open to your suggestions. |
| import org.apache.mesos.Protos.OfferID; | ||
| import org.apache.mesos.Protos.SlaveID; | ||
| import org.apache.mesos.Protos.TaskStatus; | ||
| import org.apache.mesos.Protos.*; |
There was a problem hiding this comment.
Can you revert the * import back to explicit imports? We don't like * imports.
There was a problem hiding this comment.
Sure, I didn't realize my IntelliJ settings had that enabled by default.
| @@ -98,6 +94,7 @@ public void statusUpdate(SchedulerDriver driver, TaskStatus status) { | |||
| if (status.getTaskId().getValue().contains("logviewer")) { | |||
| updateLogviewerState(status); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please delete newly introduced newlines here at line 97 and at 125, 128, and 134 as well.
| @@ -154,13 +160,34 @@ private void updateLogviewerState(TaskStatus status) { | |||
| } | |||
| } | |||
|
|
|||
| private void checkRunningLogviewerState(String logviewerZKPath) { | |||
| private void ensureZNodeExists(String logviewerZKPath) { | |||
There was a problem hiding this comment.
Since this is more for the Logviewer ZNode, can we rename it from ensureZNodeExists to ensureLogviewerZnodeExists?
| .build(); | ||
| taskStatuses.add(logviewerTaskStatus); | ||
| } | ||
| _timer.scheduleAtFixedRate(new TimerTask() { |
There was a problem hiding this comment.
align this with the rest of the code please
There was a problem hiding this comment.
This is actually aligned with the rest of the code. The diff makes it seem like it isn't. I'm taking the timer thread out of the if (_enabledLogviewerSidecar) block and scheduling it unconditionally. See https://github.com/srikanth-viswanathan/mesos-storm/blob/66dcbbe502aa3438b2a1b107d146e25de058f82c/storm/src/main/storm/mesos/MesosNimbus.java#L291 for the full file.
| // performing "explicit" reconciliation; master will respond with the latest state for all logviewer tasks | ||
| // in the framework scheduler's statusUpdate() method | ||
| List<TaskStatus> taskStatuses = new ArrayList<TaskStatus>(); | ||
| List<String> logviewerPaths = _zkClient.getChildren(_logviewerZkDir); |
There was a problem hiding this comment.
Is this the line of code that is generating the log line that you mention as the visible side-effect of this change?
There was a problem hiding this comment.
Correct. The more I think about this (and having seen the code run on our systems), I think this trace will be a nuisance, given how frequently the timer task runs (every 5 mins). I'm in favor of adding an explicit check to first test that the logviewer ZNode exists (without the error trace) before trying to get its children. Thoughts?
There was a problem hiding this comment.
Sorry for the incredibly slow response. I like this solution, please implement.
|
@JessicaLHartog Thanks for the review. I've pushed changes that address your comments. P.S.: I had originally added spaces around lines of code to add visual separation for readability. I've since removed them after your comments, but I'm curious about the reason for your suggestion. |
Reap previously launched logviewer tasks if
mesos.logviewer.sidecar.enabledis nowfalseFixes #239