You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/11/06 19:10:18 UTC
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11886
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
The presence of issues like
https://github.com/google/sanitizers/issues/757 means that LeakSanitizer
can report false positives in workloads with high concurrency.
Generally, tests will fail in the face of real leaks, but in some
scenarios, particularly those that use an ExternalMiniCluster, may
report leaks, but not actually crash (e.g. see
GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if leaks are found.
This patch updates this to only inject these errors if the test also
failed.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
1 file changed, 6 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/1
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:48:38 +0000
Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Reviewed-on: http://gerrit.cloudera.org:8080/11886
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 15 insertions(+), 5 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#2).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
The presence of issues like
https://github.com/google/sanitizers/issues/757 means that LeakSanitizer
can report false positives in workloads with high concurrency.
Generally, tests will fail in the face of real leaks, but in some test,
particularly those that use an ExternalMiniCluster, the test may report
leaks, but not actually crash (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are found.
This patch updates this to only inject these errors if the test also
failed.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
1 file changed, 6 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/2
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52
PS5, Line 52:
> Odd suggestion, seeing as you were clearly using it.
I think it's mad because the `Substitute` is in a `ifndef` block.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 01:18:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@12
PS2, Line 12: test
> Nit: tests
Ack
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: We currently inject errors into JUnit XML report if any leaks are found.
: This patch updates this to only inject these errors if the test also
: failed.
> Hmm, I don't really understand why this is safe. Are you 100% sure that a l
Ah, I like that approach much better. You're right that it isn't clear why this would be 100% safe. The paranoid part of me isn't thrilled with this either; I'll test out your suggestion to disable the checker in SuperviseThread.
Not sure about the CheckLeaks suggestion though; the issue is that the leak _is_ transient, and thus already doesn't show up after multiple loops in the CheckLeaks() call, hence the tests do _not_ fail.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:19:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc@183
PS4, Line 183: if (!has_leaks) {
> May want to make this bigger so it stands out from the report. And maybe WA
Done
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:48:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52
PS5, Line 52:
> Done
Odd suggestion, seeing as you were clearly using it.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 01:17:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#4).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 12 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/4
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc@182
PS3, Line 182: LOG(INFO) << "LeakSanitizer found false positives. Ignoring them.";
Won't this print in the normal case where there are no leaks at all? Seems like you need to detect a true->false transition here (where the initial true state from L178 doesn't count) and only LOG then.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 22:54:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52
PS5, Line 52:
> warning: using decl 'Substitute' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 01:10:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#6).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/6
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#5).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/5
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#7).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/7
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11886
to look at the new patch set (#3).
Change subject: run-test: don't report leaks that don't fail test
......................................................................
run-test: don't report leaks that don't fail test
LeakSanitizer will report a leak when allocating a string in
SuperviseThread. It's unclear why this is the case, but upon inspecting
the code, it seems like a false positive. The stack trace is as follows:
=================================================================
==93677==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 58 byte(s) in 1 object(s) allocated from:
#0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
#1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8)
#2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a)
#3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb)
#4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770)
#5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3
#6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3
#7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17
#8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0)
#9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc)
This appears to be affecting a number tests, but generally only lines #0
and #1 are present in the logs, making them difficult to debug (a
developer would have to rerun the test with specific ASAN_OPTIONS to
unwind the stacktrace more). Namely, exactly_once_writes-itest
(KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test
(untracked via Jira) all show the top of the above stack trace, and
based on the full stack trace, it seems these are all false positives.
The presence of issues like
https://github.com/google/sanitizers/issues/757 confirms that
LeakSanitizer can report false positives in workloads with high
concurrency. Generally, the test binary will return an error in the face
of real leaks, but in tests like the ones mentioned, the test may log
messages reporting leaks, but not actually return an error because the
"leak" was transient (e.g. see GenericServiceImpl::CheckLeaks).
We currently inject errors into JUnit XML report if any leaks are
reported, even for false positives, since the leak messages still find
their way into the logs. This patch updates this to only inject these
errors if the test also returned an error.
For clarity, I also threw in a log statement to
GenericServiceImpl::CheckLeaks denoting false positives.
Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
---
M build-support/run-test.sh
M src/kudu/server/generic_service.cc
2 files changed, 10 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/3
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: We currently inject errors into JUnit XML report if any leaks are found.
: This patch updates this to only inject these errors if the test also
: failed.
> Ah, I like that approach much better. You're right that it isn't clear why
Oh, so this is squarely the "false positive leak that goes away if __lsan_do_recoverable_leak_check() is called again" issue? I didn't realize LSAN still prints a leak report in that case, but I guess that's to be expected. Unfortunately I don't see a way to control the printing of the report at runtime within LSAN.
In that case, maybe your original solution is the correct one. Maybe we should also print something in GenericServiceImpl::CheckLeaks to the effect of "LSAN found false positives, ignore them".
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:29:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 2: Verified+1
The test failure seems unrelated:
TabletCopyClientSessionITest.TestStopCopyOnClientDiskFailure
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Nov 2018 20:56:09 +0000
Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 7:
Turns out IWYU was unhappy too.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 02:34:14 +0000
Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc@182
PS3, Line 182: if (i > 0) {
> Won't this print in the normal case where there are no leaks at all? Seems
Agh, you are right.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 23:03:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: Direct leak of 58 byte(s) in 1 object(s) allocated from:
: #0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92
: #1
> Oh, so this is squarely the "false positive leak that goes away if __lsan_d
Right. Todd suggested maybe patching LSAN so that it logs into a buffer instead of stdout/stderr during __lsan_do_recoverable_leak_check(), but looking into that, it seems there'd be a decent amount of plumbing to get that.
I'm somewhat conflicted because it's still uncertain whether this is the only class of leak that will not fail a test. That said, I don't think the injected JUnit errors are particularly helpful; presumably if the leak were real and worth developer attention, it would be surfaced as a test failure anyway.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 22:23:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 7: Code-Review+2
Carrying forward Adar's +2
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 03:10:20 +0000
Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@12
PS2, Line 12: test
Nit: tests
http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: We currently inject errors into JUnit XML report if any leaks are found.
: This patch updates this to only inject these errors if the test also
: failed.
Hmm, I don't really understand why this is safe. Are you 100% sure that a leak without a corresponding test failure constitutes a false positive? What evidence supports this?
Have you tried something different, such as adding a ScopedLeakCheckDisabler to the string allocation within Thread::SuperviseThread? Or, in GenericServiceImpl::CheckLeaks, either looping further or sleeping for longer in between loop cycles?
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:23:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc@183
PS4, Line 183: LOG(INFO) << "LeakSanitizer found false positives. Ignoring them.";
May want to make this bigger so it stands out from the report. And maybe WARNING instead of INFO.
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 23:50:37 +0000
Gerrit-HasComments: Yes