[kudu-CR] tablet: don't store row count in delta tracker
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 23 Feb 2018 00:21:57 + Gerrit-HasComments: No
[kudu-CR] tablet: don't store row count in delta tracker
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads; it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Reviewed-on: http://gerrit.cloudera.org:8080/9216 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h 5 files changed, 49 insertions(+), 37 deletions(-) Approvals: Kudu Jenkins: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: don't store row count in delta tracker
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@366 PS5, Line 366: Status CountRows(rowid_t *count) const override; > can we mark this final so that calls from within this class can be inlined Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@474 PS5, Line 474: // Number of rows in the rowset. > can you add a comment here that this might be unset? Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@475 PS5, Line 475: mutable std::atomic num_rows_; > let's just use int64_t here, rowid_t is kinda old and gross Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@522 PS5, Line 522: num_rows_(0), > let's use -1 as a signifier of unknown here Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@672 PS5, Line 672: rowid_t num_rows; : RETURN_NOT_OK(CountRows(&num_rows)); > this can be inside an #ifndef NDEBUG right? Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@700 PS5, Line 700: rowid_t num_rows; > same Done http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@725 PS5, Line 725: { > I don't think the extra nesting is worth making the critical section one st Done -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 06 Feb 2018 16:13:44 + Gerrit-HasComments: Yes
[kudu-CR] tablet: don't store row count in delta tracker
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9216 to look at the new patch set (#6). Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads; it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h 5 files changed, 49 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/6 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: don't store row count in delta tracker
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@366 PS5, Line 366: Status CountRows(rowid_t *count) const override; can we mark this final so that calls from within this class can be inlined instead of virtual calls? http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@474 PS5, Line 474: // Number of rows in the rowset. can you add a comment here that this might be unset? http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.h@475 PS5, Line 475: mutable std::atomic num_rows_; let's just use int64_t here, rowid_t is kinda old and gross http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@522 PS5, Line 522: num_rows_(0), let's use -1 as a signifier of unknown here http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@672 PS5, Line 672: rowid_t num_rows; : RETURN_NOT_OK(CountRows(&num_rows)); this can be inside an #ifndef NDEBUG right? http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@700 PS5, Line 700: rowid_t num_rows; same http://gerrit.cloudera.org:8080/#/c/9216/5/src/kudu/tablet/diskrowset.cc@725 PS5, Line 725: { I don't think the extra nesting is worth making the critical section one store shorter -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 06 Feb 2018 01:47:29 + Gerrit-HasComments: Yes
[kudu-CR] tablet: don't store row count in delta tracker
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9216 to look at the new patch set (#5). Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads; it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h 5 files changed, 41 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/5 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: don't store row count in delta tracker
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc@1191 PS2, Line 1191: disk rowsets th > update comment Done http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc@1290 PS2, Line 1290: RETURN_NOT_OK(cur_drs->CountRows(&num_rows)); > worried about the perf impact here -- this takes a lock and is un-inlinable Done http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc@530 PS2, Line 530: > perhaps here we should cache num_rows_ in DiskRowSet Done http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc@709 PS2, Line 709: } > same, this is a hot function Done -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 05 Feb 2018 23:36:31 + Gerrit-HasComments: Yes
[kudu-CR] tablet: don't store row count in delta tracker
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9216 to look at the new patch set (#4). Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads, it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h 5 files changed, 38 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/4 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: don't store row count in delta tracker
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9216 to look at the new patch set (#3). Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads, it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h 5 files changed, 38 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/3 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: don't store row count in delta tracker
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9216 ) Change subject: tablet: don't store row count in delta tracker .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc@1191 PS2, Line 1191: delta trackers update comment http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/compaction.cc@1290 PS2, Line 1290: RETURN_NOT_OK(cur_drs->CountRows(&num_rows)); worried about the perf impact here -- this takes a lock and is un-inlinable, and this is the hot path of a compaction. Can we cache the row count somewhere? http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc@530 PS2, Line 530: RETURN_NOT_OK(DeltaTracker::Open(rowset_metadata_, perhaps here we should cache num_rows_ in DiskRowSet http://gerrit.cloudera.org:8080/#/c/9216/2/src/kudu/tablet/diskrowset.cc@709 PS2, Line 709: DCHECK_LT(row_idx, num_rows); same, this is a hot function -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 05 Feb 2018 21:46:10 + Gerrit-HasComments: Yes
[kudu-CR] tablet: don't store row count in delta tracker
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9216 to look at the new patch set (#2). Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at instantiation-time. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads, it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc 4 files changed, 22 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/2 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] tablet: don't store row count in delta tracker
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9216 Change subject: tablet: don't store row count in delta tracker .. tablet: don't store row count in delta tracker DeltaTracker's constructor previously required a row-count that can only be obtained by first reading its parent rowset's footer from disk. While useful as a sanity check while tracking updates, it's not important for this count to be known at initialization. This patch is helpful in reducing the amount of data required from disk at startup, as one of the reasons we need to fully open some CFiles is to get this row count. This patch itself doesn't defer any reads, it just defers the requirement of the row count. This patch has no functional changes. Change-Id: I084e0944f388c22e838b017663a812b0ba77245d --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc 4 files changed, 22 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/9216/1 -- To view, visit http://gerrit.cloudera.org:8080/9216 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I084e0944f388c22e838b017663a812b0ba77245d Gerrit-Change-Number: 9216 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong