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 <[email protected]> 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; } }
