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/03/05 00:56:08 UTC
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15363
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
IMPALA-5904: (part 3) Fix more TSAN bugs
As of this commit, all backend tests are TSAN clean (or suppressions
exist for any races that are considered benign).
TSAN Fixes:
* Fixes IMPALA-9374: Possible data race in TupleDescriptor::GetLlvmStruct
* Moves the assignment of SlowDescriptor::llvm_field_idx_ from
TupleDescriptor::GetLlvmStruct to DescriptorTbl::CreateInternal
* Data race in be/src/exprs/scalar-expr-evaluator.cc:285:26
impala::ScalarExprEvaluator::GetValue(...)
* This race seems specific to data-stream-test, and does not occur
during any other tests
* I made some changes to DataStreamTest to fix the sharing of
TupleRowComparators between threads
Suppresions:
* Added a suppresion for IMPALA-9455: Possible data race in
kudu::security::InitKerberosForServer
* Added suppresions for hs2-http-test since it uses the ThriftServer, which
is known to be racey (IMPALA-9314)
Testing:
* Ran core tests
* Re-ran TSAN tests and confirmed the data races have been fixed
Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
---
M be/src/rpc/hs2-http-test.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M bin/tsan-suppressions.txt
4 files changed, 41 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/15363/1
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5460/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 08 Mar 2020 21:39:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5515/
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 11:01:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15363
to look at the new patch set (#2).
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
IMPALA-5904: (part 3) Fix more TSAN bugs
As of this commit, all backend tests are TSAN clean (or suppressions
exist for any races that are considered benign).
TSAN Fixes:
* Fixes IMPALA-9374: Possible data race in TupleDescriptor::GetLlvmStruct
* Moves the assignment of SlowDescriptor::llvm_field_idx_ from
TupleDescriptor::GetLlvmStruct to DescriptorTbl::CreateInternal
* Data race in be/src/exprs/scalar-expr-evaluator.cc:285:26
impala::ScalarExprEvaluator::GetValue(...)
* This race seems specific to data-stream-test, and does not occur
during any other tests
* I made some changes to DataStreamTest to fix the sharing of
TupleRowComparators between threads
Suppresions:
* Added a suppresion for IMPALA-9455: Possible data race in
kudu::security::InitKerberosForServer
* Added suppresions for hs2-http-test since it uses the ThriftServer, which
is known to be racey (IMPALA-9314)
Testing:
* Ran core tests
* Re-ran TSAN tests and confirmed the data races have been fixed
Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
---
M be/src/rpc/hs2-http-test.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M bin/tsan-suppressions.txt
4 files changed, 43 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/15363/2
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5460/
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Mar 2020 02:02:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2:
I thought this was working, but the following query is actually causing a crash:
select lead(-496, 81) over (order by t1.double_col desc, t1.id asc)
from functional.alltypestiny t1 where 5 = 6
union
select 794.67
from functional.alltypes t1 where 5 = 6
union all
select coalesce(10.4, int_col)
from functional.alltypes where false
The reason is that now llvm_field_idx_ is being set for every tuple, not just for the tuples where nodes call GetLlvmStruct. The issue is that for some tuples, slots have duplicate values for slot_idx_. This breaks SlotsOrderedByIdx() because (1) it allocates a vector of size = slots().size() and (2) it tries to order slots by slot_idx_, but since there a duplicate values of slot_idx_ it ends up returning a vector with a nullptr. This occurs for the query above for the tuple:
Tuple(id=0 size=0 slots=[Slot(id=0 type=DOUBLE col_path=[9] offset=-1 null=(offset=0 mask=1) slot_idx=0 field_idx=-1), Slot(id=1 type=INT col_path=[2] offset=-1 null=(offset=0 mask=1) slot_idx=0 field_idx=-1)] tuple_path=[])
Not sure how to get around this. I guess these slots aren't actually materialized so they don't need their slot_idx set. Maybe there is a way to detect this and skip these tuples? I guess if the size = 0 the slots are presumably not materialized. Or maybe just check for tuples with duplicate slot_idxs and skip them.
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Mar 2020 23:07:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2:
thinking about this some more, i think llvm_field_idx_ is always the same as slot_idx, so maybe we can get rid of llvm_field_idx_ altogether, going to try and test this out.
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Mar 2020 23:36:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 21:13:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5525/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 16:16:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5455/ : 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/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 08 Mar 2020 18:32:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has removed a vote on this change.
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5427/ : 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/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Mar 2020 01:41:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 4: Code-Review+2
There was a change several years ago to *not* send non-materialized slots to the backend. I wonder if this logic was actually a holdover from before that change.
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 22:48:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
IMPALA-5904: (part 3) Fix more TSAN bugs
As of this commit, all backend tests are TSAN clean (or suppressions
exist for any races that are considered benign).
TSAN Fixes:
* Fixes IMPALA-9374: Possible data race in TupleDescriptor::GetLlvmStruct
* Replaces the assignment of llvm_field_idx_ with slot_idx_, since
they are the same; added a DCHECK to enforce this
* Data race in be/src/exprs/scalar-expr-evaluator.cc:285:26
impala::ScalarExprEvaluator::GetValue(...)
* This race seems specific to data-stream-test, and does not occur
during any other tests
* I made some changes to DataStreamTest to fix the sharing of
TupleRowComparators between threads
Suppresions:
* Added a suppresion for IMPALA-9455: Possible data race in
kudu::security::InitKerberosForServer
* Added suppresions for hs2-http-test since it uses the ThriftServer, which
is known to be racey (IMPALA-9314)
Testing:
* Ran exhaustive tests
* Re-ran TSAN tests and confirmed the data races have been fixed
Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Reviewed-on: http://gerrit.cloudera.org:8080/15363
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
---
M be/src/rpc/hs2-http-test.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M bin/tsan-suppressions.txt
5 files changed, 47 insertions(+), 24 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/rpc/hs2-http-test.cc
File be/src/rpc/hs2-http-test.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/rpc/hs2-http-test.cc@37
PS1, Line 37: using namespace impala;
> I think you can remove this now.
Done
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/data-stream-test.cc@349
PS1, Line 349: void CreateTupleComparator(TupleRowComparator*& less_than_comparator) {
> Pass by ** to keep consistent with coding conventions.
Done
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/descriptors.cc@617
PS1, Line 617: i++
> nit: ++i
Done
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 08 Mar 2020 17:49:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5515/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 02:47:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more 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/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5559/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 05:47:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 4:
Okay, I think this is in a working state now. @Tim if you could take another look at the changes to descriptors.h and descriptors.cc that would be great.
In short, TSAN was reporting a race condition on setting the value of 'llvm_field_idx_' in TupleDescriptor::GetLlvmStruct. After taking a closer look, it looks like 'llvm_field_idx_' is always equal to 'slot_idx_' so I just replaced it with 'slot_idx_' instead. I added a DCHECK as well to ensure this.
I ran exhaustive tests with TSAN disabled, and be tests with TSAN enabled. Both test runs pass. Pre-commit passes as well.
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Mar 2020 22:07:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 1: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15363/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15363/1//COMMIT_MSG@16
PS1, Line 16: * Data race in be/src/exprs/scalar-expr-evaluator.cc:285:26
I'm confused how this worked in the first place - it seems like it'd break the sort order. Good to fix it.
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/rpc/hs2-http-test.cc
File be/src/rpc/hs2-http-test.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/rpc/hs2-http-test.cc@37
PS1, Line 37: using namespace impala;
I think you can remove this now.
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/data-stream-test.cc@349
PS1, Line 349: void CreateTupleComparator(TupleRowComparator*& less_than_comparator) {
Pass by ** to keep consistent with coding conventions.
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
http://gerrit.cloudera.org:8080/#/c/15363/1/be/src/runtime/descriptors.cc@617
PS1, Line 617: i++
nit: ++i
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Mar 2020 00:21:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15363
to look at the new patch set (#3).
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
IMPALA-5904: (part 3) Fix more TSAN bugs
As of this commit, all backend tests are TSAN clean (or suppressions
exist for any races that are considered benign).
TSAN Fixes:
* Fixes IMPALA-9374: Possible data race in TupleDescriptor::GetLlvmStruct
* Replaces the assignment of llvm_field_idx_ with slot_idx_, since
they are the same; added a DCHECK to enforce this
* Data race in be/src/exprs/scalar-expr-evaluator.cc:285:26
impala::ScalarExprEvaluator::GetValue(...)
* This race seems specific to data-stream-test, and does not occur
during any other tests
* I made some changes to DataStreamTest to fix the sharing of
TupleRowComparators between threads
Suppresions:
* Added a suppresion for IMPALA-9455: Possible data race in
kudu::security::InitKerberosForServer
* Added suppresions for hs2-http-test since it uses the ThriftServer, which
is known to be racey (IMPALA-9314)
Testing:
* Ran exhaustive tests
* Re-ran TSAN tests and confirmed the data races have been fixed
Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
---
M be/src/rpc/hs2-http-test.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M bin/tsan-suppressions.txt
5 files changed, 47 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/15363/3
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5904: (part 3) Fix more TSAN bugs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15363 )
Change subject: IMPALA-5904: (part 3) Fix more TSAN bugs
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b7b119e256085d1ba6977e1161fc658273b242
Gerrit-Change-Number: 15363
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Mar 2020 21:27:43 +0000
Gerrit-HasComments: No