You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sahil Takiar (Code Review)" <ge...@cloudera.org> on 2020/01/28 18:22:22 UTC

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs 

Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15116


Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................

IMPALA-5904: Add tsan_full option and fix several TSAN bugs

This patch adds an additional build flag -tsan_full in addition to the
existing -tsan build flag. -tsan_full is equivalent to the current -tsan
behavior, and -tsan is changed to set the ignore_noninstrumented_modules
flag to true. ignore_noninstrumented_modules causes TSAN to ignore any
modules that are not TSAN-instrumented. This is necessary to get TSAN to
play nicely with Java, since Java is not TSAN-instrumented (see
https://wiki.openjdk.java.net/display/tsan/Main and JDK-8208520). While
this might decrease the number of issues surfaced by TSAN, it drastically
decreases the amount of noise produced by TSAN because the JVM is not
running TSAN-instrumented code. Without this flag set to true, almost
every single backend test fails with the error:

WARNING: ThreadSanitizer: data race (pid=12939)
  Write of size 1 at 0x7fcbe379c4c6 by thread T31:
    #0 strncpy /mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:650 (unifiedbetests+0x1b2a4ad)
    #1 <null> <null> (libjvm.so+0x90e706)

This patch fixes various TSAN bugs (e.g. data races) reported while
running backend tests and E2E against a TSAN build (it does not make
Impala completely TSAN-clean). This patch makes the following changes:
* Fixes several bugs involving issues with updating shared variables
  between threads
* Fixes a few race conditions in test classes
* Where possible, existing locks are used to fix any data races; in cases
  where the locking logic is non-trivial, atomics are used
* There are a few places where variables are marked as 'volatile'
  presumably for synchronization purposes; TSAN flags these 'volatile'
  variables as unsafe, and according to
  https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rconc-volatile
  using 'volatile' for synchronization is dangerous; in these cases, the
  'volatile' variables are changed to 'atomic' variables
* This patch adds a suppression file (bin/tsan-suppresions.txt) similar to
  the UBSAN suppresion file (bin/ubsan-suppresions.txt)

Testing:
* Ran exhaustive tests
* Manually re-ran backend tests against a TSAN build and made sure the
reported errors are gone

Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/bufferpool/system-allocator.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/scan-range.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-test.cc
M be/src/statestore/statestore.cc
M be/src/util/runtime-profile-test.cc
M be/src/util/stopwatch.h
M bin/run-backend-tests.sh
A bin/tsan-suppressions.txt
M buildall.sh
M tests/common/environ.py
M tests/webserver/test_web_pages.py
30 files changed, 199 insertions(+), 94 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/15116/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5126/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 02:18:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15116

to look at the new patch set (#3).

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................

IMPALA-5904: Add full_tsan option and fix several TSAN bugs

This patch adds an additional build flag -full_tsan in addition to the
existing -tsan build flag. -full_tsan is equivalent to the current -tsan
behavior, and -tsan is changed to set the ignore_noninstrumented_modules
flag to true. ignore_noninstrumented_modules causes TSAN to ignore any
modules that are not TSAN-instrumented. This is necessary to get TSAN to
play nicely with Java, since Java is not TSAN-instrumented (see
https://wiki.openjdk.java.net/display/tsan/Main and JDK-8208520). While
this might decrease the number of issues surfaced by TSAN, it drastically
decreases the amount of noise produced by TSAN because the JVM is not
running TSAN-instrumented code. Without this flag set to true, almost
every single backend test fails with the error:

WARNING: ThreadSanitizer: data race (pid=12939)
  Write of size 1 at 0x7fcbe379c4c6 by thread T31:
    #0 strncpy /mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:650 (unifiedbetests+0x1b2a4ad)
    #1 <null> <null> (libjvm.so+0x90e706)

This patch fixes various TSAN bugs (e.g. data races) reported while
running backend tests and E2E against a TSAN build (it does not make
Impala completely TSAN-clean). This patch makes the following changes:
* Fixes several bugs involving issues with updating shared variables
  between threads
* Fixes a few race conditions in test classes
* Where possible, existing locks are used to fix any data races; in cases
  where the locking logic is non-trivial, atomics are used
* There are a few places where variables are marked as 'volatile'
  presumably for synchronization purposes; TSAN flags these 'volatile'
  variables as unsafe, and according to
  https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rconc-volatile
  using 'volatile' for synchronization is dangerous; in these cases, the
  'volatile' variables are changed to 'atomic' variables
* This patch adds a suppression file (bin/tsan-suppresions.txt) similar to
  the UBSAN suppresion file (bin/ubsan-suppresions.txt)

Testing:
* Ran exhaustive tests
* Ran core tests w/ ASAN build
* Manually re-ran backend tests against a TSAN build and made sure the
  reported errors are gone

Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/bufferpool/system-allocator.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/scan-range.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-test.cc
M be/src/util/runtime-profile-test.cc
M be/src/util/stopwatch.h
M bin/run-backend-tests.sh
A bin/tsan-suppressions.txt
M buildall.sh
M tests/common/environ.py
M tests/webserver/test_web_pages.py
29 files changed, 238 insertions(+), 121 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/15116/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

> (5 comments)
 > 
 > >    When I run Impala-TSAN locally, it uses the TSAN code under:
 > '/mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc'.
 > I would guess it should be using the LLVM from the toolchain?
 > Although that class doesn't seem to exist anywhere in the
 > toolchain/ dir.
 > 
 > I think that might be where the source was mounted during the
 > toolchain build?

The same thing happens with ASAN (/mnt/source/llvm/llvm-5.0.1.src-p1/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:67). So I guess its okay.

 > 
 > >    I'm a bit confused about running TSAN builds with/without the
 > -build_shared_libs option and how TSAN interacts with static vs
 > dynamic linking (trying to mimic Kudu's TSAN usage
 > https://github.com/apache/kudu/blob/master/CMakeLists.txt#L385)
 > I don't think I know any more
 > 
 > >    According to the TSAN docs, everything (including third party
 > libs) should be build with -fsanitize=thread (https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code).
 > Not sure if we are doing that in Impala?
 > We're not, the toolchain is just a regular release build.

Might consider doing this in a follow up JIRA, seems like a considerable amount of effort. It seems we can surface a lot of race conditions using TSAN without this.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2020 01:54:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG@10
PS1, Line 10: existing -tsan build flag. -tsan_full is equivalent to the current -tsan
> Is -tsan_full useful? Wonder if we should just get rid of it.
From what I have read online, running with ignore_noninstrumented_modules does mask a considerable number of race conditions:

https://github.com/google/sanitizers/issues/949

There might be some issues with 'ignore_noninstrumented_modules' as well, so it would be nice to have an option that runs without it.

I think -tsan option will be useful for automated tests, -tsan_full might be more useful for developers who can manually filter out any noise produced by third-party libs / Java.


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/reservation-tracker.h
File be/src/runtime/bufferpool/reservation-tracker.h:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/reservation-tracker.h@316
PS1, Line 316:   AtomicInt64 reservation_;
> I think these members need some explanation that they can be read without h
Done


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc
File be/src/runtime/bufferpool/system-allocator.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc@124
PS1, Line 124: #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER)
> Did you run into this bug? I think it's meant to be fixed: https://bugs.llv
hmm I thought I did, but I guess not. removed this block entirely since the LLVM bug is fixed.


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator-backend-state.cc@179
PS1, Line 179:       unique_lock<mutex> lock(lock_);
> Could this cause a deadlock if we exit from one of the points below where '
Added some docs to make it clearer. Essentially the lambda is executed in an object destructor. Since C++ destroys objects in the reverse-order they were created, the lock gets acquired after all other scoped objects (including the lock) are destroyed.


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator.h@292
PS1, Line 292:   AtomicBool has_called_wait_;  // if true, Wait() was called
> I think the writer should still hold wait_lock_?
Yeah, since it guards against concurrent calls of Coordinator::Wait that would make sense.



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2020 20:28:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5536/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 19:07:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5309/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 15:59:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 5: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 20:49:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

(5 comments)

>    When I run Impala-TSAN locally, it uses the TSAN code under: '/mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc'. I would guess it should be using the LLVM from the toolchain? Although that class doesn't seem to exist anywhere in the toolchain/ dir.

I think that might be where the source was mounted during the toolchain build?

>    I'm a bit confused about running TSAN builds with/without the -build_shared_libs option and how TSAN interacts with static vs dynamic linking (trying to mimic Kudu's TSAN usage https://github.com/apache/kudu/blob/master/CMakeLists.txt#L385)
I don't think I know any more

>    According to the TSAN docs, everything (including third party libs) should be build with -fsanitize=thread (https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code). Not sure if we are doing that in Impala?
We're not, the toolchain is just a regular release build.

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG@10
PS1, Line 10: existing -tsan build flag. -tsan_full is equivalent to the current -tsan
Is -tsan_full useful? Wonder if we should just get rid of it.


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/reservation-tracker.h
File be/src/runtime/bufferpool/reservation-tracker.h:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/reservation-tracker.h@316
PS1, Line 316:   AtomicInt64 reservation_;
I think these members need some explanation that they can be read without holding the lock (otherwise it's confusing whether code should be holding the lock or not).


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc
File be/src/runtime/bufferpool/system-allocator.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc@124
PS1, Line 124: #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER)
Did you run into this bug? I think it's meant to be fixed: https://bugs.llvm.org/show_bug.cgi?id=32968


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator-backend-state.cc@179
PS1, Line 179:       unique_lock<mutex> lock(lock_);
Could this cause a deadlock if we exit from one of the points below where 'lock_' is held?. I think maybe not since the destructor for lock_guard below runs first. But is there a way we can make the correctness more obvious?


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/coordinator.h@292
PS1, Line 292:   AtomicBool has_called_wait_;  // if true, Wait() was called
I think the writer should still hold wait_lock_?



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 21:37:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 3:

> Does ignore_noninstrumented_modules work on Linux? Looking at the
 > LLVM source, https://reviews.llvm.org/D61708 doesn't look like it's
 > been merged.
 > 
 > FWIW, it didn't exist (and certainly didn't work on Linux) when
 > Kudu's TSAN support was added, which is why we went the other
 > direction and recompile all of our dependencies with
 > -fsanitize=thread if doing a TSAN build.

@Adar, tried to dig through the LLVM code, but couldn't find any evidence that this has been fixed. FWIW I've tested it multiple times locally and the flag works, and I ran it on Jenkins and the flag works as expected.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 02:09:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15116

to look at the new patch set (#4).

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................

IMPALA-5904: Add full_tsan option and fix several TSAN bugs

This patch adds an additional build flag -full_tsan in addition to the
existing -tsan build flag. -full_tsan is equivalent to the current -tsan
behavior, and -tsan is changed to set the ignore_noninstrumented_modules
flag to true. ignore_noninstrumented_modules causes TSAN to ignore any
modules that are not TSAN-instrumented. This is necessary to get TSAN to
play nicely with Java, since Java is not TSAN-instrumented (see
https://wiki.openjdk.java.net/display/tsan/Main and JDK-8208520). While
this might decrease the number of issues surfaced by TSAN, it drastically
decreases the amount of noise produced by TSAN because the JVM is not
running TSAN-instrumented code. Without this flag set to true, almost
every single backend test fails with the error:

WARNING: ThreadSanitizer: data race (pid=12939)
  Write of size 1 at 0x7fcbe379c4c6 by thread T31:
    #0 strncpy /mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:650 (unifiedbetests+0x1b2a4ad)
    #1 <null> <null> (libjvm.so+0x90e706)

This patch fixes various TSAN bugs (e.g. data races) reported while
running backend tests and E2E against a TSAN build (it does not make
Impala completely TSAN-clean). This patch makes the following changes:
* Fixes several bugs involving issues with updating shared variables
  between threads
* Fixes a few race conditions in test classes
* Where possible, existing locks are used to fix any data races; in cases
  where the locking logic is non-trivial, atomics are used
* There are a few places where variables are marked as 'volatile'
  presumably for synchronization purposes; TSAN flags these 'volatile'
  variables as unsafe, and according to
  https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rconc-volatile
  using 'volatile' for synchronization is dangerous; in these cases, the
  'volatile' variables are changed to 'atomic' variables
* This patch adds a suppression file (bin/tsan-suppresions.txt) similar to
  the UBSAN suppresion file (bin/ubsan-suppresions.txt)

Testing:
* Ran exhaustive tests
* Ran core tests w/ ASAN build
* Manually re-ran backend tests against a TSAN build and made sure the
  reported errors are gone

Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/bufferpool/system-allocator.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/scan-range.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-test.cc
M be/src/util/runtime-profile-test.cc
M be/src/util/stopwatch.h
M bin/run-backend-tests.sh
A bin/tsan-suppressions.txt
M buildall.sh
M tests/common/environ.py
M tests/webserver/test_web_pages.py
29 files changed, 240 insertions(+), 121 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/15116/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/bufferpool/reservation-tracker.h
File be/src/runtime/bufferpool/reservation-tracker.h:

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/bufferpool/reservation-tracker.h@322
PS2, Line 322: 
> We should probably change this too for consistency (I think the same consid
Done


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc
File be/src/runtime/bufferpool/system-allocator.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc@124
PS1, Line 124: #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER)
> I assigned IMPALA-6259 to you since you're fixing it
whoops, I take that back. looks like it was fixed for ASAN in LLVM 5.0.0 but not fixed for TSAN until LLVM 6.0.0 - https://reviews.llvm.org/D35690 and https://github.com/llvm/llvm-project/commit/132689243e03b4e817aaac82d352a029216e9fee

I've updated IMPALA-6259 and the code comments



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 01:30:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5554/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2020 21:11:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/common/init.cc@423
PS3, Line 423:   // https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code),
line too long (95 > 90)


http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/runtime/bufferpool/reservation-tracker.cc
File be/src/runtime/bufferpool/reservation-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/runtime/bufferpool/reservation-tracker.cc@328
PS3, Line 328:     if (tracker->reservation_.Load() + bytes > tracker->reservation_limit_.Load()) return false;
line too long (96 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 01:28:14 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

Does ignore_noninstrumented_modules work on Linux? Looking at the LLVM source, https://reviews.llvm.org/D61708 doesn't look like it's been merged.

FWIW, it didn't exist (and certainly didn't work on Linux) when Kudu's TSAN support was added, which is why we went the other direction and recompile all of our dependencies with -fsanitize=thread if doing a TSAN build.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 22:01:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 15:59:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

> Does ignore_noninstrumented_modules work on Linux? Looking at the
 > LLVM source, https://reviews.llvm.org/D61708 doesn't look like it's
 > been merged.
 > 
 > FWIW, it didn't exist (and certainly didn't work on Linux) when
 > Kudu's TSAN support was added, which is why we went the other
 > direction and recompile all of our dependencies with
 > -fsanitize=thread if doing a TSAN build.

Maybe https://reviews.llvm.org/D28263 fixed it?

It definitely does something on my dev machine, which is running Ubuntu 16.04.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 22:30:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5125/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 01:43:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/runtime/bufferpool/reservation-tracker.cc
File be/src/runtime/bufferpool/reservation-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/15116/3/be/src/runtime/bufferpool/reservation-tracker.cc@328
PS3, Line 328:     if (tracker->reservation_.Load() + bytes > tracker->reservation_limit_.Load()) return false;
> line too long (96 > 90)
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 01:32:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/common/init.cc@423
PS2, Line 423:   // https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code),
line too long (95 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2020 20:27:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

> > Does ignore_noninstrumented_modules work on Linux? Looking at the
 > > LLVM source, https://reviews.llvm.org/D61708 doesn't look like
 > it's
 > > been merged.
 > >
 > > FWIW, it didn't exist (and certainly didn't work on Linux) when
 > > Kudu's TSAN support was added, which is why we went the other
 > > direction and recompile all of our dependencies with
 > > -fsanitize=thread if doing a TSAN build.
 > 
 > Maybe https://reviews.llvm.org/D28263 fixed it?
 > 
 > It definitely does something on my dev machine, which is running
 > Ubuntu 16.04.

mm ignore that review link. I'll try to see if I can find the patch that fixed it, but confirmed it works on my dev machine.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 22:32:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

A few things I could use input on:

* When I run Impala-TSAN locally, it uses the TSAN code under: '/mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc'. I would guess it should be using the LLVM from the toolchain? Although that class doesn't seem to exist anywhere in the toolchain/ dir.

* I'm a bit confused about running TSAN builds with/without the -build_shared_libs option and how TSAN interacts with static vs dynamic linking (trying to mimic Kudu's TSAN usage https://github.com/apache/kudu/blob/master/CMakeLists.txt#L385)

* According to the TSAN docs, everything (including third party libs) should be build with -fsanitize=thread (https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code). Not sure if we are doing that in Impala?


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 18:37:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Tim Armstrong, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15116

to look at the new patch set (#2).

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................

IMPALA-5904: Add full_tsan option and fix several TSAN bugs

This patch adds an additional build flag -full_tsan in addition to the
existing -tsan build flag. -full_tsan is equivalent to the current -tsan
behavior, and -tsan is changed to set the ignore_noninstrumented_modules
flag to true. ignore_noninstrumented_modules causes TSAN to ignore any
modules that are not TSAN-instrumented. This is necessary to get TSAN to
play nicely with Java, since Java is not TSAN-instrumented (see
https://wiki.openjdk.java.net/display/tsan/Main and JDK-8208520). While
this might decrease the number of issues surfaced by TSAN, it drastically
decreases the amount of noise produced by TSAN because the JVM is not
running TSAN-instrumented code. Without this flag set to true, almost
every single backend test fails with the error:

WARNING: ThreadSanitizer: data race (pid=12939)
  Write of size 1 at 0x7fcbe379c4c6 by thread T31:
    #0 strncpy /mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:650 (unifiedbetests+0x1b2a4ad)
    #1 <null> <null> (libjvm.so+0x90e706)

This patch fixes various TSAN bugs (e.g. data races) reported while
running backend tests and E2E against a TSAN build (it does not make
Impala completely TSAN-clean). This patch makes the following changes:
* Fixes several bugs involving issues with updating shared variables
  between threads
* Fixes a few race conditions in test classes
* Where possible, existing locks are used to fix any data races; in cases
  where the locking logic is non-trivial, atomics are used
* There are a few places where variables are marked as 'volatile'
  presumably for synchronization purposes; TSAN flags these 'volatile'
  variables as unsafe, and according to
  https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rconc-volatile
  using 'volatile' for synchronization is dangerous; in these cases, the
  'volatile' variables are changed to 'atomic' variables
* This patch adds a suppression file (bin/tsan-suppresions.txt) similar to
  the UBSAN suppresion file (bin/ubsan-suppresions.txt)

Testing:
* Ran exhaustive tests
* Ran core tests w/ ASAN build
* Manually re-ran backend tests against a TSAN build and made sure the
  reported errors are gone

Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/bufferpool/system-allocator.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/scan-range.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-test.cc
M be/src/statestore/statestore.cc
M be/src/util/runtime-profile-test.cc
M be/src/util/stopwatch.h
M bin/run-backend-tests.sh
A bin/tsan-suppressions.txt
M buildall.sh
M tests/common/environ.py
M tests/webserver/test_web_pages.py
30 files changed, 209 insertions(+), 99 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/15116/2
-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 4: Code-Review+2

Thanks for taking this on! This looks good to me. Since Tim has already +1'd, I'll go ahead and +2.


-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 18:48:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................

IMPALA-5904: Add full_tsan option and fix several TSAN bugs

This patch adds an additional build flag -full_tsan in addition to the
existing -tsan build flag. -full_tsan is equivalent to the current -tsan
behavior, and -tsan is changed to set the ignore_noninstrumented_modules
flag to true. ignore_noninstrumented_modules causes TSAN to ignore any
modules that are not TSAN-instrumented. This is necessary to get TSAN to
play nicely with Java, since Java is not TSAN-instrumented (see
https://wiki.openjdk.java.net/display/tsan/Main and JDK-8208520). While
this might decrease the number of issues surfaced by TSAN, it drastically
decreases the amount of noise produced by TSAN because the JVM is not
running TSAN-instrumented code. Without this flag set to true, almost
every single backend test fails with the error:

WARNING: ThreadSanitizer: data race (pid=12939)
  Write of size 1 at 0x7fcbe379c4c6 by thread T31:
    #0 strncpy /mnt/source/llvm/llvm-5.0.1.src-p2/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:650 (unifiedbetests+0x1b2a4ad)
    #1 <null> <null> (libjvm.so+0x90e706)

This patch fixes various TSAN bugs (e.g. data races) reported while
running backend tests and E2E against a TSAN build (it does not make
Impala completely TSAN-clean). This patch makes the following changes:
* Fixes several bugs involving issues with updating shared variables
  between threads
* Fixes a few race conditions in test classes
* Where possible, existing locks are used to fix any data races; in cases
  where the locking logic is non-trivial, atomics are used
* There are a few places where variables are marked as 'volatile'
  presumably for synchronization purposes; TSAN flags these 'volatile'
  variables as unsafe, and according to
  https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rconc-volatile
  using 'volatile' for synchronization is dangerous; in these cases, the
  'volatile' variables are changed to 'atomic' variables
* This patch adds a suppression file (bin/tsan-suppresions.txt) similar to
  the UBSAN suppresion file (bin/ubsan-suppresions.txt)

Testing:
* Ran exhaustive tests
* Ran core tests w/ ASAN build
* Manually re-ran backend tests against a TSAN build and made sure the
  reported errors are gone

Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Reviewed-on: http://gerrit.cloudera.org:8080/15116
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/bufferpool/system-allocator.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/scan-range.cc
M be/src/service/session-expiry-test.cc
M be/src/statestore/statestore-test.cc
M be/src/util/runtime-profile-test.cc
M be/src/util/stopwatch.h
M bin/run-backend-tests.sh
A bin/tsan-suppressions.txt
M buildall.sh
M tests/common/environ.py
M tests/webserver/test_web_pages.py
29 files changed, 240 insertions(+), 121 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 6
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15116/4/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/15116/4/be/src/common/init.cc@423
PS4, Line 423:   // https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code),
line too long (95 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 01:32:39 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add tsan full option and fix several TSAN bugs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add tsan_full option and fix several TSAN bugs
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/common/init.cc@423
PS1, Line 423:   // https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#non-instrumented-code),
line too long (95 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Jan 2020 18:23:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5904: Add full tsan option and fix several TSAN bugs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15116 )

Change subject: IMPALA-5904: Add full_tsan option and fix several TSAN bugs
......................................................................


Patch Set 2: Code-Review+1

(4 comments)

I had one more change requested. Assume others might want to take a look.

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15116/1//COMMIT_MSG@10
PS1, Line 10: existing -tsan build flag. -full_tsan is equivalent to the current -tsan
> From what I have read online, running with ignore_noninstrumented_modules d
Interesting, thank you!


http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/bufferpool/reservation-tracker.h
File be/src/runtime/bufferpool/reservation-tracker.h:

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/bufferpool/reservation-tracker.h@322
PS2, Line 322:   int64_t child_reservations_;
We should probably change this too for consistency (I think the same considerations apply as for the other two).


http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc
File be/src/runtime/bufferpool/system-allocator.cc:

http://gerrit.cloudera.org:8080/#/c/15116/1/be/src/runtime/bufferpool/system-allocator.cc@124
PS1, Line 124:   if (rc != 0) {
> hmm I thought I did, but I guess not. removed this block entirely since the
I assigned IMPALA-6259 to you since you're fixing it


http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/io/disk-io-mgr-stress.h
File be/src/runtime/io/disk-io-mgr-stress.h:

http://gerrit.cloudera.org:8080/#/c/15116/2/be/src/runtime/io/disk-io-mgr-stress.h@a100
PS2, Line 100: 
Reminds me of https://blog.regehr.org/archives/28 and https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html - volatile is pretty much never useful for multithreaded code



-- 
To view, visit http://gerrit.cloudera.org:8080/15116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d7ef5c228afd5882e145e6f53885b355d6c25a0
Gerrit-Change-Number: 15116
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Jan 2020 00:54:31 +0000
Gerrit-HasComments: Yes