From reviews-return-28844-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Mon Mar 28 15:48:34 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 2B23A18FA5 for ; Mon, 28 Mar 2016 15:48:34 +0000 (UTC) Received: (qmail 27025 invoked by uid 500); 28 Mar 2016 15:48:34 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 26938 invoked by uid 500); 28 Mar 2016 15:48:34 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 26072 invoked by uid 99); 28 Mar 2016 15:48:32 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 28 Mar 2016 15:48:32 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 7033628F22E; Mon, 28 Mar 2016 15:48:29 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============1739911240226499019==" MIME-Version: 1.0 Subject: Re: Review Request 45121: Implemented deletion for persistent volumes. From: Jie Yu To: Jie Yu , Joris Van Remoortere Cc: Neil Conway , mesos Date: Mon, 28 Mar 2016 15:48:29 -0000 Message-ID: <20160328154829.30713.87702@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Jie Yu X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/45121/ X-Sender: Jie Yu References: <20160326003714.6937.24212@reviews.apache.org> In-Reply-To: <20160326003714.6937.24212@reviews.apache.org> Reply-To: Jie Yu X-ReviewRequest-Repository: mesos --===============1739911240226499019== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On March 26, 2016, 12:37 a.m., Jie Yu wrote: > > src/slave/slave.cpp, line 2375 > > > > > > I would suggest we don't use CHECK here. We can just LOG(ERROR) if the deletion fails. Given the TODO above, we could leak disk space anyway. > > Neil Conway wrote: > I used `CHECK_SOME` because we use the same error-handling strategy when creating the persistent volume fails, or if the slave fails to checkpoint resources to disk successfully. Are you sure we want to handle `rmdir` errors differently? We cannot proceed when creating persistent volume fails because the subsequent 'mount' will fail. For the deletion case, if it fails, we just leak some data in the volume. The subsequent operation will not fail. Since we'll be leaking data anyway (the slave crash case), I suggest that we don't use CHECK_SOME here. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45121/#review125492 ----------------------------------------------------------- On March 27, 2016, 6:08 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45121/ > ----------------------------------------------------------- > > (Updated March 27, 2016, 6:08 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > ------- > > Prior to this commit, destroying a persistent volume would remove > the Mesos-level metadata about the volume, but wouldn't destroy > any of the volume's filesystem content. We now remove the volume > from the slave's filesystem, essentially via "rm -r". > > > Diffs > ----- > > src/slave/slave.cpp f383605a52f31d7b805ad6153adc409dcb40f83a > > Diff: https://reviews.apache.org/r/45121/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Neil Conway > > --===============1739911240226499019==--