Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )
Change subject: [tserver] Move cell and probe key size checking to prepare phase ...................................................................... Patch Set 5: (11 comments) Curious how Todd feels about this now: - On the one hand, we're reusing a bunch of legwork already done in Prepare, so the net amount of work is less. Plus we can now avoid taking row locks on invalid ops. - On the other, it is a little bit more work in Prepare, which is single-threaded. http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG@9 PS5, Line 9: inefficiency Nit: inefficient http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG@12 PS5, Line 12: these light weight checking Nit: this lightweight checking http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG@13 PS5, Line 13: lock row key Nit: locking row keys. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.h@105 PS5, Line 105: kNotCheckCellSize = 0, : kCheckCellSize = 1, Could also shorten this to YES and NO. Because it's an enum class, you have to fully qualify the values, and CheckCellSize::YES is terse and fully explanatory (vs. CheckCellSize::kCheckCellSize which is longer and duplicative). http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.h@112 PS5, Line 112: Status GetColumnSlice(const ColumnSchema& col, : CheckCellSize check_cell_size, : Slice* slice, : Status* row_status); : Status ReadColumn(const ColumnSchema& col, : CheckCellSize check_cell_size, : uint8_t* dst, : Status* row_status); These are now complicated enough that they deserve documentation. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@235 PS5, Line 235: ptr_slice->size() > FLAGS_max_cell_size_bytes)) { Nit: should be aligned to (check_cell_size == ... http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@236 PS5, Line 236: CHECK(row_status); DCHECK is probably sufficient. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@240 PS5, Line 240: // One row's column size exceed limit has been recorded in 'row_status', we will consider : // it's OK and continue to consume data in order to properly validate subsequent columns : // and rows. "After one row's column size has been found to exceed the limit and has been recorded in 'row_status', we will consider it OK and continue to consume data in order to properly validate subsequent columns and rows." http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@256 PS5, Line 256: CHECK(check_cell_size == CheckCellSize::kNotCheckCellSize || row_status); DCHECK is probably good enough here too. On second thought, if row_status != nullptr implies that we should check the cell size, maybe we don't need the enum at all? Callers should pass a non-null row_status if they want the size checked, otherwise they should pass null. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@499 PS5, Line 499: RETURN_NOT_OK(ReadColumn(col, CheckCellSize::kNotCheckCellSize, IIUC, we don't check cell sizes here because the primary key is immutable, and we had already checked it when the row was inserted. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@605 PS5, Line 605: RETURN_NOT_OK(GetColumnSlice(col, CheckCellSize::kNotCheckCellSize, Why don't we check cell sizes here? -- To view, visit http://gerrit.cloudera.org:8080/13470 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c Gerrit-Change-Number: 13470 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Sun, 28 Jul 2019 22:28:19 +0000 Gerrit-HasComments: Yes
