[GitHub] [mesos] qianzhangxa commented on a change in pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.

[GitBox <gi...@apache.org>]

qianzhangxa commented on a change in pull request #388:
URL: https://github.com/apache/mesos/pull/388#discussion_r654460042



##########
File path: src/linux/cgroups.cpp
##########
@@ -1425,16 +1424,15 @@ class TasksKiller : public Process<TasksKiller>
       const PID<TasksKiller>& pid)
   {
     // Cancel the freeze operation.
-    // TODO(jieyu): Wait until 'future' is in DISCARDED state before
-    // starting retry.
     future.discard();
 
-    // We attempt to kill the processes before we thaw again,
-    // due to a bug in the kernel. See MESOS-1758 for more details.
-    // We thaw the cgroup before trying to freeze again to allow any
-    // pending signals to be delivered. See MESOS-1689 for details.
-    // This is a short term hack until we have PID namespace support.
-    return Future<bool>(true)
+    // Wait until the freeze is cancelled, and then attempt to kill the
+    // processes before we thaw again, due to a bug in the kernel. See
+    // MESOS-1758 for more details.  We thaw the cgroup before trying to freeze
+    // again to allow any pending signals to be delivered. See MESOS-1689 for
+    // details.  This is a short term hack until we have PID namespace support.
+    return future
+      .recover([](const Future<Nothing>&){ return Future<Nothing>(Nothing()); })

Review comment:
       I think you intention is to wait until `future` is in `DISCARDED` state before starting retry, right? But if that happens, will we just return from here without calling the 3 methods below (i.e. no retry)?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org