You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2019/05/29 11:02:37 UTC
[kudu-CR] [tablet] Support accurate count of rows
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13456
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
For the MemRowSet and DeltaMemStore, there are real-time counters;
For the DiskRowSet, the counters are persisted in the RowSetMetadata.
At the same time, the counters of MemRowSet will be transfered to
DiskRowSet after flushing, and the counters of DeltaMemStore will be
merged to DiskRowSet after flushing.
Formula:
Tablet = MemRowSet + [ DiskRowSet ... ]
| |
\ \__
\__ RowSetMetadata + DeltaMemStore
counter | |
\__ \__
counter counter
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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-test.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 271 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/1
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#8).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 392 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/8
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/1/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/1/src/kudu/tablet/compaction-test.cc@a48
PS1, Line 48:
It seems there is something wrong with iwyu.
2019-05-29 21:46:08,999 INFO: Checking 22 file(s)...
>>> Fixing #includes in '/mnt/ddb/2/helif/apache/kudu/src/kudu/tablet/compaction-test.cc'
@@ -47,7 +47,6 @@
#include "kudu/consensus/opid.pb.h"
#include "kudu/consensus/opid_util.h"
#include "kudu/fs/block_id.h"
-#include "kudu/fs/block_manager.h"
#include "kudu/fs/fs_manager.h"
#include "kudu/fs/log_block_manager.h"
#include "kudu/gutil/casts.h"
IWYU would have edited 1 files on your behalf.
CMakeFiles/iwyu.dir/build.make:57: recipe for target 'CMakeFiles/iwyu' failed
make[3]: *** [CMakeFiles/iwyu] Error 1
CMakeFiles/Makefile2:392: recipe for target 'CMakeFiles/iwyu.dir/all' failed
make[2]: *** [CMakeFiles/iwyu.dir/all] Error 2
CMakeFiles/Makefile2:399: recipe for target 'CMakeFiles/iwyu.dir/rule' failed
make[1]: *** [CMakeFiles/iwyu.dir/rule] Error 2
Makefile:305: recipe for target 'iwyu' failed
make: *** [iwyu] Error 2
hzhelifu@db-87:/mnt/ddb/2/helif/apache/kudu/build/release$ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description: Debian GNU/Linux 8.9 (jessie)
Release: 8.9
Codename: jessie
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 29 May 2019 13:50:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#6).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Jjust like step2.
8.For the historical tablets:
When a negative number is returned, it means it is a historical
tablet. And a historical tablet can return a positive number of
live rows after compaction and etc.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 304 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/6
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#10).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 393 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/10
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 10
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 7:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 7.Compact:
: Just like step2.
:
: __counter
> > need to be compacted?) so naturally the user will turn to other
You mean rereplicating all the tablets on a tserver? It's not an explicit "feature" per se, but you can do it by stopping a tserver and waiting for the rereplication timeout (default 5 minutes) to elapse. After that, all of the tserver's replicas will begin rereplication elsewhere.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@235
PS7, Line 235: void DeleteRows(RowSet* rowset, int n_rows) {
This can chain to the new DeleteRows:
DeleteRows(rowset, n_rows, 0);
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1209
PS7, Line 1209: ASSERT_NO_FATAL_FAILURE();
NO_FATALS() in new code.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1251
PS7, Line 1251: > >
>> (the old syntax is pre-C++11)
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1252
PS7, Line 1252: push_back
Use emplace_back for all new code. And std::move the DRS shared pointers.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/delta_tracker.cc@721
PS7, Line 721: Old
old
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
PS7:
Please update the AppendBlock docs to explain what live_row_count means.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@138
PS7, Line 138: real-time
Did you mean 'live' here? Not sure what 'real-time' is supposed to convey in context.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@139
PS7, Line 139: // It's only supported for the newly created ones, not for the ancient ones.
I'd add that when false, the 'live_row_count' in every RowSetDataPB is incorrect and should be ignored.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@140
PS7, Line 140: support_live_row_count
Nit: supports_live_row_count.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/rowset_metadata.cc
File src/kudu/tablet/rowset_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/rowset_metadata.cc@124
PS7, Line 124: pb.has_live_row_count())
I don't think you need this second check; the default value of live_row_count() is 0, and support_live_row_count() is all we should need to know whether live_row_count() can be trusted.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.h@84
PS7, Line 84: bool support_live_row_count,
supports_live_row_count
Elsewhere too.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@154
PS7, Line 154: true
To clarify the meaning of true/false in call sites, add an annotation:
/*supports_live_row_count=*/true
Elsewhere too.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@272
PS7, Line 272: bool support_live_row_count)
Do you actually need it to be configurable via the constructor? Couldn't we always set it to true in the constructor, then get the correct on-disk value when we call LoadFromSuperBlock?
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
PS7:
Can you add a tablet copy test to check that:
1. If the tablet didn't support live row counting, it still doesn't after a copy
2, If it did, the value is the same after the copy.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tserver/tablet_copy_client.cc@355
PS7, Line 355: bool is_support_live_row_count = false;
: if (remote_superblock_->has_support_live_row_count()) {
: is_support_live_row_count = remote_superblock_->support_live_row_count();
: }
You can just do:
is_support_live_row_count = remote_superblock_->support_live_row_count();
The default value of the field (if it's missing) will be false, which is the behavior you want.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 23:48:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 8.For the historical tablets:
: When a negative number is returned, it means it is a historical
: tablet. And a historical tablet can return a positive number of
: live rows after compaction and etc.
> need to be compacted?) so naturally the user will turn to other
> means, such as rereplicating all the tablets from one tserver at a
> time.
BTW, it doesn't seem to have that feature yet, does it?
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 12:08:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#9).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 392 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/9
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc@255
PS11, Line 255: /*supports_live_row_count=*/ false
> Done
Can you restore this? Like Andrew observed, it's a tiny amount of space, and an exception for the master tablet draws attention for no good reason.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 13
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 05 Jun 2019 20:49:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 9:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 7.Compact:
: Just like step2.
:
: __counter
> Ah, I see your point: because tablet copy operates at the level of data blo
^_^
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
PS7:
> Also in RollingDiskRowSetWriter.
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@272
PS7, Line 272: const TabletDataState& tablet_data_state,
> Ah, right. I forgot to remove this suggestion after reviewing that; sorry.
it doesn't matter :)
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h
File src/kudu/tserver/tablet_copy-test-base.h:
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h@113
PS9, Line 113: virtual void GenerateTestData() {
> Why does this need to be virtual?
I want to inject the modification of the 'supports_live_row_count' through polymorphism characteristic. Please look at L147 in tserver/tablet_copy_client-test.cc. And I think that's a simplest way to support your first comment in https://gerrit.cloudera.org/#/c/13456/7/src/kudu/tserver/tablet_copy_client.cc
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 04:57:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc@255
PS11, Line 255: /*supports_live_row_count=*/ false
> Can you restore this? Like Andrew observed, it's a tiny amount of space, an
Done
http://gerrit.cloudera.org:8080/#/c/13456/13/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13456/13/src/kudu/tablet/tablet.cc@1922
PS13, Line 1922: *count += tmp;
> nit: can remove this.
sorry, a mistake:(
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 13
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 06 Jun 2019 00:42:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 11
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 06:14:52 +0000
Gerrit-HasComments: No
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 2:
(3 comments)
I don
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG@10
PS2, Line 10: For the DiskRowSet, the counters are persisted in the RowSetMetadata.
I don't think this new persistent state is necessary; we should be able to establish its value at runtime using:
1. CFileSet::CountRows(): a single IO the first time this is called, after which it's just a read from the in-memory CFile footer.
2. Visit all REDO DeltaFiles, finish initializing them if necessary, and add/substract reinsert_count/delete_count from the delta stats respectively.
3. Visit the DMS and add the value of the new counter you added. Or maintain a full-fledged DeltaStats for each DMS.
Once all the initialization is out of the way, this aggregation is just in-memory operations.
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/deltamemstore.h
File src/kudu/tablet/deltamemstore.h:
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/deltamemstore.h@187
PS2, Line 187: // Number of rows deleted in DMS.
: int32_t deleted_row_count_;
: // Number of rows reinserted in DMS.
: int32_t reinserted_row_count_;
Could you use delta_stats_ instead?
Though in either case I think these need to be atomic; DMS mutations can happen concurrently. Maybe DeltaStats should be templatized on a trait that indicates whether to use locking or not. See arena.h or scoped_refptr.h for some inspiration here.
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/diskrowset.h@378
PS2, Line 378: virtual uint64_t GetValidRowCount() const override;
Name it CountRowsValid() to clarify the symmetry with CountRows. May have to return a Status and take an IOContext if any of its subcalls perform initialization.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 29 May 2019 19:01:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#13).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1. At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2. Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3. Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4. If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5. MinorCompact:
No influence since any insertions or deletions come from memory;
6. MajorCompact:
No influence, just like step5;
7. Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 389 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/13
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 13
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#4).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadataS will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the counter3 will be added to the
counter2 of RowSetMetadata;
5.MinorCompact:
no influence since any insertions or deletions come from memory;
6.MajorCompact:
no influence, just like step5;
7.Compact:
just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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-test.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 271 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/4
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 4:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG
Commit Message:
PS4:
Nice description and ASCII art.
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@7
PS4, Line 7: [tablet] Support accurate count of rows
How will this work for existing tablets? AFAICT all of the count changes are incremental, which doesn't work at all if the persistent counters are set to 0 in existing tablets.
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@12
PS4, Line 12: When MRS is being flushed, it will be attached to the RowSetTree,
Technically the MRS was part of the RowSetTree _before_ being flushed too. Since you're trying to explain how this all comes together to produce a tablet-level count, you might want to mention the RowSetTree (and aggregation across rowsets, including the MRS) before step 1.
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@14
PS4, Line 14: RowSetMetadataS
RowSetMetadatas
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@21
PS4, Line 21: counter3
Nit: value of counter3
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/compaction.cc@1168
PS4, Line 1168: // If the REDO is empty, it should not be a DELETE.
: if (new_redos_head == nullptr) {
: out->increase_valid_row_count();
: }
Are you absolutely sure that new_redos_head != nullptr means the row was deleted? Maybe you can add some debug-only assertions to enforce that?
Also, consider moving this to L1163 as an else statement.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h@270
PS4, Line 270: int32_t CountRowsValid() const;
Nit: add an empty line after.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h@368
PS4, Line 368: // For the old DMS which is being flushed, swap its valid row count.
: // NOTE: this value will be merged to RowSetMetadata after finished.
Nit: rewrite as:
// Number of valid (i.e. not deleted) rows for a DMS that is currently being flushed.
// When the flush completes, this is merged into the RowSetMetadata and reset.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/deltamemstore.h
File src/kudu/tablet/deltamemstore.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/deltamemstore.h@187
PS4, Line 187: // Number of valid rows in DMS.
Technically the DMS doesn't allow reinserts, so this value can only be 0 or negative. Perhaps it would be clearer if instead if tracked the number of _deleted_ rows, and was subtracted from the RowSetMetadata value at flush time?
Same goes for the DeltaTracker: perhaps it should track the number of deletions and that value subtracted in DiskRowSet::CountRowsValid().
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h@225
PS4, Line 225: void increase_valid_row_count() { valid_row_count_++; }
The dependency between this and FlushCompactionInput isn't pretty. Instead, how about doing this as part of AppendBlock? You can modify AppendBlock to take an additional argument which is the number of valid rows in the block.
Also, could you implement the same functionality in DiskRowSetWriter? It's only a few lines of code, and the discrepancy is a little weird right now.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h@282
PS4, Line 282: in this loop.
Nit: "for the current DRS being written."
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset-test.cc
File src/kudu/tablet/memrowset-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset-test.cc@825
PS4, Line 825: ASSERT_EQ(4, mrs->CountRowsValid());
ASSERT that it's 0 before GenerateTestData?
Can you also ASSERT that the count changes (or doesn't) correctly when:
1. Rows are deleted.
2. Rows are reinserted.
3. Rows are mutated (no change).
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h@258
PS4, Line 258: virtual uint64_t CountRowsValid() const override {
Google Style Guide says to use int64_t (and assertions) to enforce that a number is never negative rather than using an unsigned int: https://google.github.io/styleguide/cppguide.html#Integer_Types
This applies to the entire change.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/metadata.proto@50
PS4, Line 50: optional int32 valid_row_count = 10;
Even though nothing in RowSetDataPB is documented, could you doc the meaning of this field?
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@228
PS4, Line 228: void IncreaseValidRowCount(int32_t valid_row_count);
Nit: Increment is more clear (i.e. we already have methods that start with Increment() and accept negative values).
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@230
PS4, Line 230: int32_t CountRowsValid() const;
Name this valid_row_count() since it's a simple accessor. https://google.github.io/styleguide/cppguide.html#Function_Names
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc
File src/kudu/tablet/rowset_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc@115
PS4, Line 115: if (pb.has_valid_row_count()) {
: valid_row_count_ = pb.valid_row_count();
: }
Likewise, move this to the end; it looks like it's part of "Load redo delta files".
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc@143
PS4, Line 143: if (valid_row_count_ != 0) {
: pb->set_valid_row_count(valid_row_count_);
: }
Nit: move this to the end of the function. As is it looks like it has to do with "Write Delta Files" but it doesn't.
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/tablet.h@350
PS4, Line 350: // Returns the valid row count in this tablet.
Doc that "valid" means rows that haven't been deleted. Do you think "live" would be clearer?
This applies to your other new functions where "valid" is mentioned.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 31 May 2019 07:13:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 8.For the historical tablets:
: When a negative number is returned, it means it is a historical
: tablet. And a historical tablet can return a positive number of
: live rows after compaction and etc.
> 1.BTW, do you think this is better than a superblock-level capability that'
New fields are cheap, especially booleans (~2 bytes). And it might be nice to formalize a "capabilities" feature for superblocks anyway.
So the question is: what's the user impact of doing a tablet-level capability thing vs. the rowset-level thing? Ultimately, the user either cares about the live row count, or doesn't care:
- If the user doesn't care, it doesn't matter what we do.
- If the user does care, he/she will probably want to get the feature working on all their tablets. Waiting for all tablets to recompact fully is hard/impossible (what if a rowset just doesn't need to be compacted?) so naturally the user will turn to other means, such as rereplicating all the tablets from one tserver at a time.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 04:03:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#7).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
30 files changed, 378 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/7
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/compaction-test.cc@1206
PS10, Line 1206: ASSERT_NO_FATAL_FAILURE();
> Since you've wrapped function calls that can ASSERT with NO_FATALS(), you d
Done
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/diskrowset.cc@205
PS10, Line 205: if (rowset_metadata_->tablet_metadata()->supports_live_row_count() &&
> After reviewing this a couple times, I think it'd be clearer if IncrementLi
Yea, very good advice!
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 10
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 05:50:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#11).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
No influence since any insertions or deletions come from memory;
6.MajorCompact:
No influence, just like step5;
7.Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 386 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/11
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 11
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/compaction-test.cc@1206
PS10, Line 1206: ASSERT_NO_FATAL_FAILURE();
Since you've wrapped function calls that can ASSERT with NO_FATALS(), you don't need the standalone ASSERT_NO_FATAL_FAILURE() calls.
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/13456/10/src/kudu/tablet/diskrowset.cc@205
PS10, Line 205: if (rowset_metadata_->tablet_metadata()->supports_live_row_count() &&
After reviewing this a couple times, I think it'd be clearer if IncrementLiveRows() were responsible for checking supports_live_row_count(), and if it's false, just ignoring the request.
That applies here as well as in DeltaTracker.
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h
File src/kudu/tserver/tablet_copy-test-base.h:
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h@113
PS9, Line 113: virtual void GenerateTestData() {
> I want to inject the modification of the 'supports_live_row_count' through
Ah, missed that addition. Thanks for clarifying.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 10
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 05:24:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@7
PS4, Line 7: [tablet] Support accurate count of rows
> Would it be possible to add a capability to the tablet superblock to track
Done
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 8.For the historical tablets:
: When a negative number is returned, it means it is a historical
: tablet. And a historical tablet can return a positive number of
: live rows after compaction and etc.
> This isn't as clear as it could be. It's important to add that a tablet wil
1.BTW, do you think this is better than a superblock-level capability that's set for new tablets?
--> It's possible to add a new field to the superblock to indicate that the newly created tablets support real live row count. But, 1) it will take up an additional field, 2) it becomes a little bit difficult to 'historical' tablet to support this feature;
2.Do you really expect tablets to eventually be fully recompacted such that every DRS has a correct live row count? I wouldn't expect that out of most workloads: often times there are sections of keyspace that are "ancient" and are probably already fully compacted.
--> Hmm, maybe you are right. So, do we need to close the door for the 'ancient' tablets?
3.-1 into Status::NotSupported?
--> Yea, good idea!
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 03:05:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#5).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree.
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5.MinorCompact:
no influence since any insertions or deletions come from memory;
6.MajorCompact:
no influence, just like step5;
7.Compact:
just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 291 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/5
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG@10
PS2, Line 10: For the DiskRowSet, the counters are persisted in the RowSetMetadata.
> The reason why using a new persistent state here is:
OK, that's a pretty compelling counter-argument. And the size of the new persistent state is low. I was just worried about the complexity of having multiple pieces of semi-redundant state in different places.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 30 May 2019 03:13:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 14
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 06 Jun 2019 00:52:11 +0000
Gerrit-HasComments: No
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#14).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1. At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2. Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3. Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4. If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5. MinorCompact:
No influence since any insertions or deletions come from memory;
6. MajorCompact:
No influence, just like step5;
7. Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 388 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/14
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 14
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 7:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 7.Compact:
: Just like step2.
:
: __counter
> You mean rereplicating all the tablets on a tserver? It's not an explicit "
No, I mean a replicated tablet can enable the live row counting feature from an ancient tablet? As far as I know the replication is done by copying blocks, regardless of the number of live rows. That will affect the code L361 in tablet_copy_client.cc.
Ah, I think I got it: "1. If the tablet didn't support live row counting, it still doesn't after a copy" ^_^
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@235
PS7, Line 235: void DeleteRows(RowSet* rowset, int n_rows) {
> This can chain to the new DeleteRows:
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1209
PS7, Line 1209: ASSERT_NO_FATAL_FAILURE();
> NO_FATALS() in new code.
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1251
PS7, Line 1251: > >
> >> (the old syntax is pre-C++11)
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/compaction-test.cc@1252
PS7, Line 1252: push_back
> Use emplace_back for all new code. And std::move the DRS shared pointers.
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/delta_tracker.cc@721
PS7, Line 721: Old
> old
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
PS7:
> Please update the AppendBlock docs to explain what live_row_count means.
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@138
PS7, Line 138: real-time
> Did you mean 'live' here? Not sure what 'real-time' is supposed to convey i
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@139
PS7, Line 139: // It's only supported for the newly created ones, not for the ancient ones.
> I'd add that when false, the 'live_row_count' in every RowSetDataPB is inco
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/metadata.proto@140
PS7, Line 140: support_live_row_count
> Nit: supports_live_row_count.
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/rowset_metadata.cc
File src/kudu/tablet/rowset_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/rowset_metadata.cc@124
PS7, Line 124: pb.has_live_row_count())
> I don't think you need this second check; the default value of live_row_cou
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.h@84
PS7, Line 84: bool support_live_row_count,
> supports_live_row_count
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@154
PS7, Line 154: true
> To clarify the meaning of true/false in call sites, add an annotation:
Done
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@272
PS7, Line 272: bool support_live_row_count)
> Do you actually need it to be configurable via the constructor? Couldn't we
No, please look at the code L361 in tablet_copy_client.cc.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
PS7:
> Can you add a tablet copy test to check that:
Ok, i will try.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tserver/tablet_copy_client.cc@355
PS7, Line 355: bool is_support_live_row_count = false;
: if (remote_superblock_->has_support_live_row_count()) {
: is_support_live_row_count = remote_superblock_->support_live_row_count();
: }
> You can just do:
Done
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 01:42:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 9:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 7.Compact:
: Just like step2.
:
: __counter
> No, I mean a replicated tablet can enable the live row counting feature fro
Ah, I see your point: because tablet copy operates at the level of data blocks and WAL segments, it's difficult to reconstruct the live row count.
This only leaves full recompaction as an option, which I don't think is realistic. So I guess we'll have to resign ourselves to only having this feature in truly new tablets.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
PS7:
> Done
Also in RollingDiskRowSetWriter.
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/7/src/kudu/tablet/tablet_metadata.cc@272
PS7, Line 272: const TabletDataState& tablet_data_state,
> No, please look at the code L361 in tablet_copy_client.cc.
Ah, right. I forgot to remove this suggestion after reviewing that; sorry.
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h
File src/kudu/tserver/tablet_copy-test-base.h:
http://gerrit.cloudera.org:8080/#/c/13456/9/src/kudu/tserver/tablet_copy-test-base.h@113
PS9, Line 113: virtual void GenerateTestData() {
Why does this need to be virtual?
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 04 Jun 2019 04:09:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 8.For the historical tablets:
: When a negative number is returned, it means it is a historical
: tablet. And a historical tablet can return a positive number of
: live rows after compaction and etc.
> New fields are cheap, especially booleans (~2 bytes). And it might be nice
Done
http://gerrit.cloudera.org:8080/#/c/13456/6/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/6/src/kudu/tablet/delta_tracker.h@269
PS6, Line 269: // Count the number of deleted rows in this Tracker.
> The comment suggests that it'll count up the rows in the entire tracker, (i
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h@258
PS4, Line 258: virtual int64_t CountLiveRows() const override {
> Having done this, could you add DCHECKs to the various counting/subtraction
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@228
PS4, Line 228: // Note: positive number means live rows and negative number means deleted rows.
> Sorry, I didn't mean to suggest that you should rename methods like these "
Done
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 11:59:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 13: Code-Review+1
(2 comments)
One last nit and this looks good to go!
http://gerrit.cloudera.org:8080/#/c/13456/13/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13456/13/src/kudu/tablet/tablet.cc@1922
PS13, Line 1922: *count += tmp;
nit: can remove this.
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@412
PS11, Line 412: }
:
: TEST_F(TabletCopyClientTest, TestSupportsLiveRowCount) {
: ASSERT_OK(StartCopy());
:
> No, seems impossible. The 'BootstrapTablet' doesn't help. Maybe L786~L806 i
OK. We should make sure that we have some test coverage for the correctness of the live row count after tablet copying, though this can be in a follow-up patch when the row count is more externally available (e.g. from an RPC).
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 13
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 05 Jun 2019 16:42:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 11:
(8 comments)
Some nits and a testing suggestion, overall LGTM.
http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG
Commit Message:
PS11:
Some important limitations to be aware of are that the counts will be inaccurate if either:
- we are on a Kudu version that supports counting, we downgrade to a version without counts, write some rows, and then upgrade again to a version with counts, or
- we tablet copy between a version that supports counts and a version that doesn't support counts, we write rows to the one that doesn't support counts, and then upgrade it
I can't think of a clean way around either of these, but I think both of these are edge cases that we generally don't expect anyway. Still important to be aware of them.
http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG@15
PS11, Line 15: 1.
nit: could you add a space between the numbers and the sentences and align them all like that? E.g.
1. At the beginning...
reinsert) into...
2. Next, MRS...
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc@255
PS11, Line 255: /*supports_live_row_count=*/ true,
Is it important to have this for the SysCatalog tablet? I suppose it's not a whole lot of extra space, but I'm curious what the rationale for it is.
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h@370
PS11, Line 370:
: // When the flush completes, this is merged into the RowSetMetadata a
nit: strange newline formatting?
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto@138
PS11, Line 138: // Whether the tablet supports live row counting.
: // It's only supported for the newly created ones, not for the ancient ones.
: // When false, the 'live_row_count' in every RowSetDataPB is incorrect and
: // should be ignored.
nit: Could you reword this slightly?
"Whether the table supports counting live rows. If false, 'live_row_count' may be inaccurate and should be ignored."
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc@1919
PS11, Line 1919: int64_t tmp = 0;
: RETURN_NOT_OK(comps->memrowset->CountLiveRows(&tmp));
: *count += tmp;
: for (const shared_ptr<RowSet>& rowset : comps->rowsets->all_rowsets()) {
: RETURN_NOT_OK(rowset->CountLiveRows(&tmp));
: *count += tmp;
: }
nit: This will update `count` even if this method fails, which can be surprising. Perhaps rewrite it as:
int64_t ret = 0;
int64_t tmp = 0;
RETURN_NOT_OK(comps->memrowset->CountLiveRows(&ret));
for (const shared_ptr<RowSet>& rowset : comps->rowset->all_rowset()) {
RETURN_NOT_OK(rowset->CountLiveRows(&tmp);
ret += tmp;
}
*count = ret;
return Status::OK();
so it only gets updated if returning successfully
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@147
PS11, Line 147: virtual void GenerateTestData() {
: Random rand(SeedRandom());
: NO_FATALS(tablet_replica_->tablet_metadata()->
: set_supports_live_row_count_for_tests(rand.Next() % 2));
: NO_FATALS(TabletCopyTest::GenerateTestData());
: }
nit: Would be good to add a comment description.
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@412
PS11, Line 412: TEST_F(TabletCopyClientTest, TestSupportsLiveRowCount) {
: ASSERT_OK(StartCopy());
: ASSERT_EQ(tablet_replica_->tablet_metadata()->supports_live_row_count(),
: meta_->supports_live_row_count());
: }
Do you know whether it would be easy to verify that row counts as well? I am thinking perhaps something like L773 in tool_action_perf.cc to bootstrap a tablet (with no replica) and verifying the counts match.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 11
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 05 Jun 2019 03:21:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#12).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1. At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2. Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3. Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4. If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5. MinorCompact:
No influence since any insertions or deletions come from memory;
6. MajorCompact:
No influence, just like step5;
7. Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 389 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/12
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 12
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 11:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG
Commit Message:
PS11:
> Some important limitations to be aware of are that the counts will be inacc
Yea, that's true. And thank you for reminding.^_^
http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG@15
PS11, Line 15: 1.
> nit: could you add a space between the numbers and the sentences and align
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc@255
PS11, Line 255: /*supports_live_row_count=*/ true,
> Is it important to have this for the SysCatalog tablet? I suppose it's not
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h@370
PS11, Line 370:
: // When the flush completes, this is merged into the RowSetMetadata a
> nit: strange newline formatting?
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto@138
PS11, Line 138: // Whether the tablet supports live row counting.
: // It's only supported for the newly created ones, not for the ancient ones.
: // When false, the 'live_row_count' in every RowSetDataPB is incorrect and
: // should be ignored.
> nit: Could you reword this slightly?
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc@1919
PS11, Line 1919: int64_t tmp = 0;
: RETURN_NOT_OK(comps->memrowset->CountLiveRows(&tmp));
: *count += tmp;
: for (const shared_ptr<RowSet>& rowset : comps->rowsets->all_rowsets()) {
: RETURN_NOT_OK(rowset->CountLiveRows(&tmp));
: *count += tmp;
: }
> nit: This will update `count` even if this method fails, which can be surpr
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@147
PS11, Line 147: virtual void GenerateTestData() {
: Random rand(SeedRandom());
: NO_FATALS(tablet_replica_->tablet_metadata()->
: set_supports_live_row_count_for_tests(rand.Next() % 2));
: NO_FATALS(TabletCopyTest::GenerateTestData());
: }
> nit: Would be good to add a comment description.
Done
http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@412
PS11, Line 412: TEST_F(TabletCopyClientTest, TestSupportsLiveRowCount) {
: ASSERT_OK(StartCopy());
: ASSERT_EQ(tablet_replica_->tablet_metadata()->supports_live_row_count(),
: meta_->supports_live_row_count());
: }
> Do you know whether it would be easy to verify that row counts as well? I a
No, seems impossible. The 'BootstrapTablet' doesn't help. Maybe L786~L806 in tool_action_perf.cc can help, which means we need to scan the live rows and then sum it. But, I don't think it's necessary -_-
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 11
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 05 Jun 2019 06:07:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/2//COMMIT_MSG@10
PS2, Line 10: For the DiskRowSet, the counters are persisted in the RowSetMetadata.
> I don't think this new persistent state is necessary; we should be able to
The reason why using a new persistent state here is:
1.The 'CountRows()' returns the number of rows, but it doesn't distinguish whether it is alive or not(deleted); And I think the accurate number of rows is very important to query engines like impala;
2.The first heartbeat will bring centralized disk reading and footer parsing load(key index file and redo delta files) after the node restarts;
3.For the cold data, it's unnecessary to initialize it, and it can help to release disk io and etc.
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/deltamemstore.h
File src/kudu/tablet/deltamemstore.h:
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/deltamemstore.h@187
PS2, Line 187: // Number of rows deleted in DMS.
: int32_t deleted_row_count_;
: // Number of rows reinserted in DMS.
: int32_t reinserted_row_count_;
> Could you use delta_stats_ instead?
Ok.
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/2/src/kudu/tablet/diskrowset.h@378
PS2, Line 378: virtual uint64_t GetValidRowCount() const override;
> Name it CountRowsValid() to clarify the symmetry with CountRows. May have t
Done
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 30 May 2019 02:01:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#2).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
For the MemRowSet and DeltaMemStore, there are real-time counters;
For the DiskRowSet, the counters are persisted in the RowSetMetadata.
At the same time, the counters of MemRowSet will be transfered to
DiskRowSet after flushing, and the counters of DeltaMemStore will be
merged to DiskRowSet after flushing.
Formula:
Tablet = MemRowSet + [ DiskRowSet ... ]
| |
\ \__
\__ RowSetMetadata + DeltaMemStore
counter | |
\__ \__
counter counter
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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-test.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 271 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/2
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13456
to look at the new patch set (#3).
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
1.At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2.Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadataS will be created
and every one has persistent counter2;
3.Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4.If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the counter3 will be added to the
counter2 of RowSetMetadata;
5.MinorCompact:
no influence;
6.MajorCompact:
I'll make it up tomorrow.
7.Compact:
just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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-test.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
23 files changed, 271 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/13456/3
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
[tablet] Support accurate count of rows
A tablet is consisted of one MRS and a group of DRS. And the
group of DRS makes up a RowSetTree. Thus, the number of live
rows in a tablet comes from MRS and RowSetTree. At the same
time, a new capability is added to the tablet superblock to
ensure that only newly created tablets enable the capability.
1. At the beginning, new rows will be written (insert/delete/
reinsert) into MRS and counter1 holds the number of rows;
2. Next, MRS is being flushed to DRS (flush1):
When MRS is being flushed, it will be attached to the RowSetTree,
so we can count rows there (still counter1);
When MRS is flushed to DRS, new RowSetMetadatas will be created
and every one has persistent counter2;
3. Then, there will be updates to DRS and they will be accumulated
in DMS. counter3 holds the number of rows;
4. If DMS is flushed (flush2):
When DMS is being flushed, it will be attached to the Redo list,
and its counter3 will be swapped to DeltaTracker;
When DMS is flushed to Redo, the value of counter3 will be added
to the counter2 of RowSetMetadata;
5. MinorCompact:
No influence since any insertions or deletions come from memory;
6. MajorCompact:
No influence, just like step5;
7. Compact:
Just like step2.
__counter2(persistent)
/
|
__RowSetMetadata __counter3
/ /
flush1 | |
[MRS] -------> [ DRS ... ] -- Undo + Redo + DMS
| ^ |
\__ |______|
counter1 flush2
Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Reviewed-on: http://gerrit.cloudera.org:8080/13456
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.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.cc
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/metadata.proto
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/ts_tablet_manager.cc
31 files changed, 388 insertions(+), 33 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 15
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [tablet] Support accurate count of rows
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 4:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG
Commit Message:
PS4:
> Nice description and ASCII art.
Thanks^_^
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@7
PS4, Line 7: [tablet] Support accurate count of rows
> How will this work for existing tablets? AFAICT all of the count
> changes are incremental, which doesn't work at all if the
> persistent counters are set to 0 in existing tablets.
Yea, it's a big problem indeed. And it seems there is no good way to backward compatible. Well, maybe we can give our users some suggestions:
1) reload the data for the dimension tables;
2) roll the fact tables, then the old tablets that have not counters will be deleted;
3) if users don't like above ideas, that means they are not interested in this feature.
What do you think?
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@12
PS4, Line 12: When MRS is being flushed, it will be attached to the RowSetTree,
> Technically the MRS was part of the RowSetTree _before_ being flushed too.
Done
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@14
PS4, Line 14: RowSetMetadataS
> RowSetMetadatas
Done
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@21
PS4, Line 21: counter3
> Nit: value of counter3
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/compaction.cc@1168
PS4, Line 1168: // If the REDO is empty, it should not be a DELETE.
: if (new_redos_head == nullptr) {
: out->increase_valid_row_count();
: }
> Are you absolutely sure that new_redos_head != nullptr means the row was de
Yes, because L938~L940 show it's a DELETE when new_redos_head != nullptr. :)
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h@270
PS4, Line 270: int32_t CountRowsValid() const;
> Nit: add an empty line after.
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/delta_tracker.h@368
PS4, Line 368: // For the old DMS which is being flushed, swap its valid row count.
: // NOTE: this value will be merged to RowSetMetadata after finished.
> Nit: rewrite as:
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/deltamemstore.h
File src/kudu/tablet/deltamemstore.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/deltamemstore.h@187
PS4, Line 187: // Number of valid rows in DMS.
> Technically the DMS doesn't allow reinserts, so this value can only be 0 or
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h@225
PS4, Line 225: void increase_valid_row_count() { valid_row_count_++; }
> The dependency between this and FlushCompactionInput isn't pretty. Instead,
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/diskrowset.h@282
PS4, Line 282: in this loop.
> Nit: "for the current DRS being written."
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset-test.cc
File src/kudu/tablet/memrowset-test.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset-test.cc@825
PS4, Line 825: ASSERT_EQ(4, mrs->CountRowsValid());
> ASSERT that it's 0 before GenerateTestData?
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h@258
PS4, Line 258: virtual uint64_t CountRowsValid() const override {
> Google Style Guide says to use int64_t (and assertions) to enforce that a n
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/metadata.proto@50
PS4, Line 50: optional int32 valid_row_count = 10;
> Even though nothing in RowSetDataPB is documented, could you doc the meanin
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@228
PS4, Line 228: void IncreaseValidRowCount(int32_t valid_row_count);
> Nit: Increment is more clear (i.e. we already have methods that start with
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@230
PS4, Line 230: int32_t CountRowsValid() const;
> Name this valid_row_count() since it's a simple accessor. https://google.gi
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc
File src/kudu/tablet/rowset_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc@115
PS4, Line 115: if (pb.has_valid_row_count()) {
: valid_row_count_ = pb.valid_row_count();
: }
> Likewise, move this to the end; it looks like it's part of "Load redo delta
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.cc@143
PS4, Line 143: if (valid_row_count_ != 0) {
: pb->set_valid_row_count(valid_row_count_);
: }
> Nit: move this to the end of the function. As is it looks like it has to do
Done
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/tablet.h@350
PS4, Line 350: // Returns the valid row count in this tablet.
> Doc that "valid" means rows that haven't been deleted. Do you think "live"
Done
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 31 May 2019 17:37:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@32
PS6, Line 32: Jjust
Just
http://gerrit.cloudera.org:8080/#/c/13456/6//COMMIT_MSG@33
PS6, Line 33: 8.For the historical tablets:
: When a negative number is returned, it means it is a historical
: tablet. And a historical tablet can return a positive number of
: live rows after compaction and etc.
This isn't as clear as it could be. It's important to add that a tablet will only transition from "historical" to "regular" (i.e. with a real live row count) after every single DRS has been rewritten. Meaning, as long as there's one historical DRS, the overall tablet live row count will be -1. BTW, do you think this is better than a superblock-level capability that's set for new tablets? Do you really expect tablets to eventually be fully recompacted such that every DRS has a correct live row count? I wouldn't expect that out of most workloads: often times there are sections of keyspace that are "ancient" and are probably already fully compacted.
Speaking of -1, could we have the chain of functions involved in getting the live row count return a Status so that we can convert a -1 into Status::NotSupported? I think it'll be clearer for callers.
http://gerrit.cloudera.org:8080/#/c/13456/6/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/13456/6/src/kudu/tablet/delta_tracker.h@269
PS6, Line 269: // Count the number of deleted rows in this Tracker.
The comment suggests that it'll count up the rows in the entire tracker, (i.e. also in the UNDOs and REDOs). But that's not accurate: it only returns deleted rows in the current DMS as well as in a flushing DMS (if one exists).
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/memrowset.h@258
PS4, Line 258: virtual int64_t CountLiveRows() const override {
> Done
Having done this, could you add DCHECKs to the various counting/subtraction functions that ensure that the row count never becomes negative?
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/13456/4/src/kudu/tablet/rowset_metadata.h@228
PS4, Line 228: // Note: positive number means live rows and negative number means deleted rows.
> Done
Sorry, I didn't mean to suggest that you should rename methods like these "Increment", but rather to "IncrementValidRowCount" instead of "IncreaseValidRowCount". Without the "ValidRowCount" part the name is too ambiguous.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Sun, 02 Jun 2019 22:06:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] Support accurate count of rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13456/4//COMMIT_MSG@7
PS4, Line 7: [tablet] Support accurate count of rows
> > How will this work for existing tablets? AFAICT all of the count
Would it be possible to add a capability to the tablet superblock to track whether to use this feature or not? Every RowSetMetadata points back to its owning TabletMetadata so in theory all the non-performance-critical areas of code (i.e. non MRS/DMS) could evaluate the capability to decide whether to honor an IncrementValidCount or ignore it. Likewise, Tablet::CountRowsValid() could look at it to decide whether to do the aggregation or to return an error (-1 or something). As long as you can ensure that only newly created tablets enable the capability, it could work.
--
To view, visit http://gerrit.cloudera.org:8080/13456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412
Gerrit-Change-Number: 13456
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 31 May 2019 18:11:58 +0000
Gerrit-HasComments: Yes