Hi Ben, Thanks for raising this!
My thinking for grouping the changes together in a single review is basically what AlexR said: I agree with doing "one thing per patch", but I felt like a header cleanup was sufficiently close to CHECK cleanup that they could be grouped together. If there's consensus that folks would rather see such changes separated, I'm happy to do that in the future. Re: the <stout/check.hpp> change, my mistake -- although I confess I didn't realize we have a strict "no dependence on transitive includes" policy. Is that documented anywhere? Anyway, RRs here: https://reviews.apache.org/r/47112 avoids depending on transitively included headers https://reviews.apache.org/r/47113 replaces "using namespace process;" with more fine-grained "using" statements https://reviews.apache.org/r/47114 fixes angle bracket style. Thanks, Neil On Sun, May 8, 2016 at 10:28 AM, Alex R <al...@apache.org> wrote: > I agree that "atomic patches" (those that do one thing per patch) are a good > thing because they simplify navigating history, do blame and bisect. But how > to define that "one thing"? Some people would say, that a new feature is one > thing, and if introducing a feature requires some refactoring, it should be > done in the same patch, so that motivation for the refactoring is clear. On > the other hand there will be folks who would say that putting all > loosely-coupled changes like refactoring, tests, protobuf update, > implementation into the same patch makes it hardly reviewable and > complicates finding problems via blame / bisect. > > Let's take a step back and think, why we need to read commit history in the > first place. Based on my experience, I see several cases: > * Searching which patch introduced a bug, a regression, or some peculiar > behaviour. > * Learning how a feature was implemented and why. > > That's why it is important to separate functional changes from cleanups. > > However, do we still want to separate different sorts of cleanups as well or > squash them together to avoid the churn? If I search for a bug and see a > style fix patch, I simply skip. I'd rather prefer to have one single patch > for all style fixes than a tiny patch for each type of cleanup. > > Regarding r/46827/, you are right that doing audit of include and using > sections was not the primary intention, but I considered it fine to include > those changes since they were not making the code "worse" (modulo removing > <stout/check.hpp> which was a mistake). > > On 8 May 2016 at 02:38, Benjamin Mahler <bmah...@apache.org> wrote: >> >> Hm.. any reason that unrelated headers were touched and the using >> statement was removed in this patch? >> >> My concern with mixing unrelated changes within a single patch is that >> patches become less precise. If one reads the patch there is additional >> overhead in understanding what is related to the goal of the change and what >> is not. I know it's a small example here but I see value in being >> disciplined about this regardless of patch size. >> >> The other concern is that the reviewer of this patch has to review these >> two additional changes: >> 1. How does the header audit look? Anything else need added or removed? >> 2. How does the 'using' audit look? Anything else need added or removed? >> >> (1) and (2) could be done together in a single patch. As in turns out, the >> header audit looks like it has a few issues, but I'm guessing the reviewer >> glossed over it because the point of this change was CHECK_READY :) >> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used >> -<glog/logging.h> is not present but LOG is used >> -<stout/nothing.hpp> is absent but Nothing is used >> -<process/process.hpp> is absent but process::Process / process::wait / >> process::terminate are used. >> >> Then for the 'using' audit, we now avoid pulling in all of the process:: >> namespace in favor of finer-grained using statements. >> >> On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote: >>> >>> Repository: mesos >>> Updated Branches: >>> refs/heads/master 78f6101cc -> 4f9040db6 >>> >>> >>> Replaced CHECK with CHECK_READY. >>> >>> Also removes some unused header includes. >>> >>> Review: https://reviews.apache.org/r/46827/ >>> >>> >>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo >>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db >>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db >>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db >>> >>> Branch: refs/heads/master >>> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310 >>> Parents: 78f6101 >>> Author: Neil Conway <neil.con...@gmail.com> >>> Authored: Mon May 2 13:05:56 2016 +0200 >>> Committer: Alexander Rukletsov <al...@apache.org> >>> Committed: Mon May 2 13:05:56 2016 +0200 >>> >>> ---------------------------------------------------------------------- >>> src/zookeeper/contender.cpp | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> ---------------------------------------------------------------------- >>> >>> >>> >>> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp >>> ---------------------------------------------------------------------- >>> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp >>> index 4b1cc65..5eb4b58 100644 >>> --- a/src/zookeeper/contender.cpp >>> +++ b/src/zookeeper/contender.cpp >>> @@ -14,26 +14,23 @@ >>> // See the License for the specific language governing permissions and >>> // limitations under the License >>> >>> -#include <set> >>> #include <string> >>> >>> #include <mesos/zookeeper/contender.hpp> >>> #include <mesos/zookeeper/detector.hpp> >>> #include <mesos/zookeeper/group.hpp> >>> >>> +#include <process/check.hpp> >>> #include <process/defer.hpp> >>> #include <process/dispatch.hpp> >>> #include <process/future.hpp> >>> #include <process/id.hpp> >>> >>> -#include <stout/check.hpp> >>> #include <stout/lambda.hpp> >>> #include <stout/option.hpp> >>> -#include <stout/some.hpp> >>> >>> using namespace process; >>> >>> -using std::set; >>> using std::string; >>> >>> namespace zookeeper { >>> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel() >>> >>> void LeaderContenderProcess::cancelled(const Future<bool>& result) >>> { >>> - CHECK(candidacy.isReady()); >>> + CHECK_READY(candidacy); >>> LOG(INFO) << "Membership cancelled: " << candidacy.get().id(); >>> >>> // Can be called as a result of either withdraw() or server side >>> >> >
