Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/9751 )
Change subject: [experimental] Clang Tidy Diff trial balloon ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9751/1//COMMIT_MSG@22 PS1, Line 22: The main change is to find a way to get the > I see - I always buildall, so I never noticed that myself. The backstory on this (which is somewhat redundant given Phil's comment) is that I have had some changes that passed Gerrit review that then failed on GVO due to clang-tidy. The code changes passed the jenkins.impala.io's pre-review-test job (which doesn't do clang-tidy). My experience is that failure during GVO is the worst possible time. There are a few observations: A. I should have run clang-tidy. B. pre-review-test should do clang-tidy C. There should be an automated process that flags clang-tidy problems on Gerrit reviews. "A" is true. I should have run clang-tidy. clang-tidy-1604 on jenkins.impala.io takes about 20 minutes (17-22 minutes or so). My guess is that this fits pretty well with how long it takes on my development machine. I would like "A" to be faster. Processing only the changed files speeds this up. On Kudu, clang-tidy-diff.py usually runs in a couple minutes. When something takes longer and ties up a development environment, developers get lazy. We fixed "B" by adding a gerrit-verify-dryrun-external, which does exactly what GVO does without the +1 verify. This is great. I use it all the time. I recently did some code changes on Kudu, and "C" is a very nice place to be. Quite a few clang-tidy issues won't be caught by reviewers. The clang-tidy issue that hit me on Kudu was this: string long_string(INT_MAX, 'a'); Clang-tidy fails due to the string initialization being too big. I didn't know this check existed. "C" found it within an hour of posting to Gerrit without tying up my development system. It could have failed much later. We should have "C" for Impala. I'm agnostic about how this happens. The script in this code change has logic to interpret the diff and post to Gerrit. We can use that or adapt it to the existing buildall.sh system (which doesn't sound too hard if the diff is similar). Jim, you listed #3 before #1,#2. I want to make sure I'm not misinterpreting your preferences. What would you see as steps we need to take for #3? I don't see very much cost to having two parallel systems, so long as they are labeled appropriately. Having some experience from actual code changes trumps any of my arm-chair analysis about which is better. That's why I'm in favor of merging this before figuring out whether it can fully replace the existing system. In other words, I put strong emphasis on the "if we can disable one" part of #3 when I put it last. I'm also generally ok with having both for an extended period of time (or forever). Making the two systems distinct and clear to developers is important. A developer should know whether this is a diff-based clang-tidy or a full clang-tidy, and the scripts should be clear about what they do and how they are named. -- To view, visit http://gerrit.cloudera.org:8080/9751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7 Gerrit-Change-Number: 9751 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 26 Mar 2018 01:38:04 +0000 Gerrit-HasComments: Yes
