[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..


KUDU-1692: tie various reader memtrackers to the tablet tracker

Without the explicit connection, these memtrackers are all parented to the
root tracker, whose Release() behavior is rather expensive.

There's also some refactoring here:
- It was frustrating to pass the tablet's main and DMS memtrackers into
  various places, so there's a TabletMemTrackers struct to encapsulate them.
- There's some pass-by-value and std::move() for objects that are either
  large or may be large one day (e.g. ReaderOptions).
- CFileSet, MemRowSet and DeltaMemStore now follow the object construction
  idiom that we use elsewhere: a static method that creates the object and
  may return failure.

Note: this was originally https://gerrit.sjc.cloudera.com/#/c/7029. It's
since been rebased and all of the additional tracking was dropped.

Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Reviewed-on: http://gerrit.cloudera.org:8080/4708
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_mem_trackers.h
M src/kudu/tools/tool_action_local_replica.cc
29 files changed, 490 insertions(+), 278 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..

KUDU-1692: tie various reader memtrackers to the tablet tracker

Without the explicit connection, these memtrackers are all parented to the
root tracker, whose Release() behavior is rather expensive.

There's also some refactoring here:
- It was frustrating to pass the tablet's main and DMS memtrackers into
  various places, so there's a TabletMemTrackers struct to encapsulate them.
- There's some pass-by-value and std::move() for objects that are either
  large or may be large one day (e.g. ReaderOptions).
- CFileSet, MemRowSet and DeltaMemStore now follow the object construction
  idiom that we use elsewhere: a static method that creates the object and
  may return failure.

Note: this was originally https://gerrit.sjc.cloudera.com/#/c/7029. It's
since been rebased and all of the additional tracking was dropped.

Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_mem_trackers.h
M src/kudu/tools/tool_action_local_replica.cc
29 files changed, 490 insertions(+), 278 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4708/3/src/kudu/tablet/deltamemstore.cc
File src/kudu/tablet/deltamemstore.cc:

Line 65: HeapBufferAllocator::Get(), parent_tracker)),
> this one shouldn't be a move?
My mistake; it should be a move as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4708/3/src/kudu/tablet/deltamemstore.cc
File src/kudu/tablet/deltamemstore.cc:

Line 65: HeapBufferAllocator::Get(), parent_tracker)),
this one shouldn't be a move?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..

KUDU-1692: tie various reader memtrackers to the tablet tracker

Without the explicit connection, these memtrackers are all parented to the
root tracker, whose Release() behavior is rather expensive.

There's also some refactoring here:
- It was frustrating to pass the tablet's main and DMS memtrackers into
  various places, so there's a TabletMemTrackers struct to encapsulate them.
- There's some pass-by-value and std::move() for objects that are either
  large or may be large one day (e.g. ReaderOptions).
- CFileSet, MemRowSet and DeltaMemStore now follow the object construction
  idiom that we use elsewhere: a static method that creates the object and
  may return failure.

Note: this was originally https://gerrit.sjc.cloudera.com/#/c/7029. It's
since been rebased and all of the additional tracking was dropped.

Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_mem_trackers.h
M src/kudu/tools/tool_action_local_replica.cc
29 files changed, 490 insertions(+), 278 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..

KUDU-1692: tie various reader memtrackers to the tablet tracker

Without the explicit connection, these memtrackers are all parented to the
root tracker, whose Release() behavior is rather expensive.

There's also some refactoring here:
- It was frustrating to pass the tablet's main and DMS memtrackers into
  various places, so there's a TabletMemTrackers struct to encapsulate them.
- There's some pass-by-value and std::move() for objects that are either
  large or may be large one day (e.g. ReaderOptions).
- CFileSet, MemRowSet and DeltaMemStore now follow the object construction
  idiom that we use elsewhere: a static method that creates the object and
  may return failure.

Note: this was originally https://gerrit.sjc.cloudera.com/#/c/7029. It's
since been rebased and all of the additional tracking was dropped.

Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_mem_trackers.h
M src/kudu/tools/tool_action_local_replica.cc
29 files changed, 490 insertions(+), 278 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1692: tie various reader memtrackers to the tablet tracker

[Adar Dembo (Code Review)]

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: KUDU-1692: tie various reader memtrackers to the tablet tracker
..

KUDU-1692: tie various reader memtrackers to the tablet tracker

Without the explicit connection, these memtrackers are all parented to the
root tracker, whose Release() behavior is rather expensive.

There's also some refactoring here:
- It was frustrating to pass the tablet's main and DMS memtrackers into
  various places, so there's a TabletMemTrackers struct to encapsulate them.
- There's some pass-by-value and std::move() for objects that are either
  large or may be large one day (e.g. ReaderOptions).
- CFileSet, MemRowSet and DeltaMemStore now follow the object construction
  idiom that we use elsewhere: a static method that creates the object and
  may return failure.

Note: this was originally https://gerrit.sjc.cloudera.com/#/c/7029. It's
since been rebased and all of the additional tracking was dropped.

Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_mem_trackers.h
M src/kudu/tools/tool_action_local_replica.cc
29 files changed, 468 insertions(+), 260 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I40ea59dc5d70c8ec935f9d96bcdb914c1d23ec5a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon