You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by mz...@apache.org on 2019/08/14 03:40:33 UTC
[mesos] 01/03: Fixed and improved `Sorter::remove` function.
This is an automated email from the ASF dual-hosted git repository.
mzhu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit cb7b70db813a8e4d2628e9283e25780cd0ce11b0
Author: Meng Zhu <mz...@mesosphere.io>
AuthorDate: Wed Aug 7 16:11:20 2019 -0700
Fixed and improved `Sorter::remove` function.
There is a bug in the remove() function where after collapsing
and turning an internal node into an leaf node, the new node's
position needs to updated in the parent's children list.
See MESOS-9930.
This patch fixes the bug and also refactored the function logic.
Review: https://reviews.apache.org/r/71255
---
src/master/allocator/mesos/sorter/drf/sorter.cpp | 23 +++++++++-------------
.../allocator/mesos/sorter/random/sorter.cpp | 23 +++++++++-------------
2 files changed, 18 insertions(+), 28 deletions(-)
diff --git a/src/master/allocator/mesos/sorter/drf/sorter.cpp b/src/master/allocator/mesos/sorter/drf/sorter.cpp
index 40397f7..b7da3ff 100644
--- a/src/master/allocator/mesos/sorter/drf/sorter.cpp
+++ b/src/master/allocator/mesos/sorter/drf/sorter.cpp
@@ -207,15 +207,14 @@ void DRFSorter::remove(const string& clientPath)
}
if (current->children.empty()) {
+ // Simply delete the current node if it has no children.
parent->removeChild(current);
delete current;
} else if (current->children.size() == 1) {
- // If `current` has only one child that was created to
- // accommodate inserting `clientPath` (see `DRFSorter::add()`),
- // we can remove the child node and turn `current` back into a
+ // If `current` has only one virtual node ".", we can collapse
+ // and remove that node, and turn `current` back into a
// leaf node.
Node* child = *(current->children.begin());
-
if (child->name == ".") {
CHECK(child->isLeaf());
CHECK(clients.contains(current->path));
@@ -223,20 +222,16 @@ void DRFSorter::remove(const string& clientPath)
current->kind = child->kind;
current->removeChild(child);
+ delete child;
// `current` has changed kind (from `INTERNAL` to a leaf,
- // which might be active or inactive). Hence we might need to
- // change its position in the `children` list.
- if (current->kind == Node::INTERNAL) {
- CHECK_NOTNULL(current->parent);
-
- current->parent->removeChild(current);
- current->parent->addChild(current);
- }
+ // which might be active or inactive). We need to change
+ // its position in the `children` list.
+ CHECK_NOTNULL(current->parent);
+ current->parent->removeChild(current);
+ current->parent->addChild(current);
clients[current->path] = current;
-
- delete child;
}
}
diff --git a/src/master/allocator/mesos/sorter/random/sorter.cpp b/src/master/allocator/mesos/sorter/random/sorter.cpp
index b480aee..3e93a4a 100644
--- a/src/master/allocator/mesos/sorter/random/sorter.cpp
+++ b/src/master/allocator/mesos/sorter/random/sorter.cpp
@@ -201,15 +201,14 @@ void RandomSorter::remove(const string& clientPath)
}
if (current->children.empty()) {
+ // Simply delete the current node if it has no children.
parent->removeChild(current);
delete current;
} else if (current->children.size() == 1) {
- // If `current` has only one child that was created to
- // accommodate inserting `clientPath` (see `RandomSorter::add()`),
- // we can remove the child node and turn `current` back into a
+ // If `current` has only one virtual node ".", we can collapse
+ // and remove that node, and turn `current` back into a
// leaf node.
Node* child = *(current->children.begin());
-
if (child->name == ".") {
CHECK(child->isLeaf());
CHECK(clients.contains(current->path));
@@ -217,20 +216,16 @@ void RandomSorter::remove(const string& clientPath)
current->kind = child->kind;
current->removeChild(child);
+ delete child;
// `current` has changed kind (from `INTERNAL` to a leaf,
- // which might be active or inactive). Hence we might need to
- // change its position in the `children` list.
- if (current->kind == Node::INTERNAL) {
- CHECK_NOTNULL(current->parent);
-
- current->parent->removeChild(current);
- current->parent->addChild(current);
- }
+ // which might be active or inactive). We need to change
+ // its position in the `children` list.
+ CHECK_NOTNULL(current->parent);
+ current->parent->removeChild(current);
+ current->parent->addChild(current);
clients[current->path] = current;
-
- delete child;
}
}