Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20025 )
Change subject: IMPALA-12193: Fix TSAN failures for DataCacheTest.SetReadOnly ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/20025/1/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/20025/1/be/src/runtime/io/data-cache.cc@325 PS1, Line 325: std::unique_lock<percpu_rwlock> lock(lock_); > Curious about the position of the lock here, does it really work? The other threads in other functions get the lock in shared mode, so getting the lock here forces this thread to wait until all the shared mode threads have exited the critical section. The lock doesn't protect readonly_ as a variable. I'll add a comment here http://gerrit.cloudera.org:8080/#/c/20025/1/be/src/runtime/io/data-cache.cc@329 PS1, Line 329: lock_ > Do we still need the lock after using atomic? Good point, we can skip the lock for revoking read only. -- To view, visit http://gerrit.cloudera.org:8080/20025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc507f09b9c093a9034d601d8e7d37976bd0433e Gerrit-Change-Number: 20025 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 09 Jun 2023 21:45:42 +0000 Gerrit-HasComments: Yes
