You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/12/14 09:10:35 UTC

[kudu-CR] external mini cluster: check for leaks before killing servers 

Todd Lipcon has uploaded a new change for review.

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

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 90 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5501/6/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:

Line 108: #ifndef LSAN_ENABLED
> Nit: I think it'd be simpler to elide LSAN_ENABLED and just move the code b
The problem then is that the '#else' would apply to the outer #if instead of the inner one.


Line 113:   resp->set_found_leaks(__lsan_do_recoverable_leak_check());
> I don't suppose we could also report more information on the leak itself? S
__lsan_do_recoverable_leak_check dumps the stacks to the output in the same way that the normal end-of-process one does


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 95 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................


external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Reviewed-on: http://gerrit.cloudera.org:8080/5501
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 97 insertions(+), 14 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 87 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 97 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5501/6/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:

Line 108: #ifndef LSAN_ENABLED
> The problem then is that the '#else' would apply to the outer #if instead o
I see. Yeah you'd have to duplicate the resp->set_success(false) call in two places: once if !__has_feature(address_sanitizer), and once if !defined(__has_feature). That's uglier.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has uploaded a new patch set (#2).

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 89 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................


Patch Set 6: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5501/6/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:

Line 108: #ifndef LSAN_ENABLED
Nit: I think it'd be simpler to elide LSAN_ENABLED and just move the code below into the nested if as appropriate.


Line 113:   resp->set_found_leaks(__lsan_do_recoverable_leak_check());
I don't suppose we could also report more information on the leak itself? Stack traces, things like that?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] external mini cluster: check for leaks before killing servers

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: external_mini_cluster: check for leaks before killing servers
......................................................................

external_mini_cluster: check for leaks before killing servers

This improves the ExternalMiniCluster when running with LeakSanitizer
enabled so that, before killing subprocesses, it runs a leak check.
Without this, it's hard for us to know if there are any leaks in code
paths that are covered only by EMC-based tests.

Tested by manually inserting a leak into the tablet server and verifying
that an EMC test failed.

Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/generic_service.cc
M src/kudu/server/generic_service.h
M src/kudu/server/server_base.proto
M src/kudu/util/debug/leak_annotations.h
6 files changed, 88 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/5501/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5501
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b6c38281e35e8300e849f92e91d6955a8d735bf
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot