You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/09/13 01:03:52 UTC

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint 

Hello Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed once out of 1000 dist-test runs. I
could not get it to fail locally at all; maybe I'm doing something wrong?

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
18 files changed, 66 insertions(+), 117 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3387/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed 2% of dist-test runs, though I could
not get it to fail locally at all.

I also removed a few test-only overrides of table creation timeout. They are
detrimental now that the default admin operation timeout is 30s (it used to
be 5s).

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Reviewed-on: http://gerrit.cloudera.org:8080/4394
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/ts_itest-base.h
A src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
22 files changed, 176 insertions(+), 144 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

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

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

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed 2% of dist-test runs, though I could
not get it to fail locally at all.

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
A src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
20 files changed, 171 insertions(+), 136 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

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

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

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed once out of 1000 dist-test runs. I
could not get it to fail locally at all; maybe I'm doing something wrong?

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
19 files changed, 128 insertions(+), 135 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

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

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

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed 2% of dist-test runs, though I could
not get it to fail locally at all.

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/test_workload.cc
A src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
21 files changed, 171 insertions(+), 140 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 6:

Build Started http://104.196.14.100/job/kudu-gerrit/3424/

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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3388/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/integration-tests/external_mini_cluster-test.cc
File src/kudu/integration-tests/external_mini_cluster-test.cc:

Line 128: // any crashes.
might make more sense to put this in a different test case... this test in theory is supposed to be just testing the test infra, not actual functionality/bugs


Line 142:       MonoDelta::FromMicroseconds(100));
maybe you'd hit the race more with a super low value like 1us?

maybe even start multiple threads if you really wanna make a solid stress test of this?


Line 154:   work.StopAndJoin();
might be more fruitful if you set the log rolling threshold to be really low, and inserted much more data, since the particular MRS-related test is a per-segment thing.


Line 156:   // Restart the cluster. Nothing should crash.
how about an AssertNoCrashes before the restart, too?


http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:

Line 237:     LOG(FATAL) <<
should we consider DFATAL here? would it actually cause incorrect behavior to hit this in a real cluster?


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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/3409/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 6: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/3432/

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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

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

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

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed 2% of dist-test runs, though I could
not get it to fail locally at all.

I also removed a few test-only overrides of table creation timeout. They are
detrimental now that the default admin operation timeout is 30s (it used to
be 5s).

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/ts_itest-base.h
A src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
22 files changed, 176 insertions(+), 144 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/3417/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 6: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/integration-tests/external_mini_cluster-test.cc
File src/kudu/integration-tests/external_mini_cluster-test.cc:

Line 142
> At first I tried 1us and 10 checker threads, but there was too much load an
sure, that seems fine.


Line 154
> AFAICT the number of log segments is orthogonal to the number of times tabl
k, my mistake


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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 6: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/3425/

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

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

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/3414/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

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

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

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................

KUDU-1090: relax MemTracker uniqueness constraint

This patch relaxes the MemTracker uniqueness constraint to avoid certain
rare crashes (e.g. web UI takes a reference to a MemRowSet tracker during
Tablet::RewindSchemaForBootstrap). Details on these crashes can be found in
the associated bug report.

More specifically, the constraint is preserved, but it is only enforced
during FindTracker() or FindOrCreateTracker(). This way it is there for
MemTracker users that really should avoid duplicates (e.g. enforcing that
every tablet has just one MemTracker for all of its DeltaMemStores), but out
of the way for everyone else. With this change, we can remove the various
hacks and workarounds that were sprinkled in various tests.

Without the patch, the new test failed 2% of dist-test runs, though I could
not get it to fail locally at all.

Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_failover-itest.cc
A src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
20 files changed, 168 insertions(+), 136 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea8e3d383878e829188eaca65d639bb44d8b8146
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1090: relax MemTracker uniqueness constraint

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

Change subject: KUDU-1090: relax MemTracker uniqueness constraint
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/integration-tests/external_mini_cluster-test.cc
File src/kudu/integration-tests/external_mini_cluster-test.cc:

Line 128: // any crashes.
> might make more sense to put this in a different test case... this test in 
Will do.


Line 142:       MonoDelta::FromMicroseconds(100));
> maybe you'd hit the race more with a super low value like 1us?
At first I tried 1us and 10 checker threads, but there was too much load and cluster.Restart() timed out in 20% of runs. So I raised the poll period to 1ms, and now 23/1000 runs fail with the MemTracker error, but I still can't reliably repro it locally.

Do you think that's good enough?


Line 154:   work.StopAndJoin();
> might be more fruitful if you set the log rolling threshold to be really lo
AFAICT the number of log segments is orthogonal to the number of times tablet::RewindTabletForBootstrap is called (which is just once).


Line 156:   // Restart the cluster. Nothing should crash.
> how about an AssertNoCrashes before the restart, too?
Sure.


http://gerrit.cloudera.org:8080/#/c/4394/2/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:

Line 237:     LOG(FATAL) <<
> should we consider DFATAL here? would it actually cause incorrect behavior 
It would lead to incorrect memory accounting, I think, but maybe that's the worst of it.

I'll change to DFATAL and return the first found tracker.


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

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