Yingchun Lai 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 6: (11 comments) 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: inefficient > Nit: inefficient Done http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG@12 PS5, Line 12: this lightweight checking t > Nit: this lightweight checking Done http://gerrit.cloudera.org:8080/#/c/13470/5//COMMIT_MSG@13 PS5, Line 13: locking row > Nit: locking row keys. Done 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: Status ReadIssetBitmap(const uint8_t** bitmap); : Status ReadNullBitmap > Could also shorten this to YES and NO. Because it's an enum class, you have Done http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.h@112 PS5, Line 112: // Same as above, but store result in 'dst'. : Status ReadColumn(const ColumnSchema& col, uint8_t* dst, Status* row_status); : // Some column which is non-nullable has allocated a cell to row data in : // RowOperationsPBEncoder::Add, even if its data is useless (i.e. set to : // NULL), we have to consume data in order to properly validate subsequent : // columns and rows. : Status ReadColumnAndDiscard(const ColumnSchema& col); : bool HasNext() const; > These are now complicated enough that they deserve documentation. Done 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: "value too large for column '$0' ($1 bytes, maximum is $2 bytes)", > Nit: should be aligned to (check_cell_size == ... Done http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@236 PS5, Line 236: col.name(), ptr_slice->size(), FLAGS_max_cell_size_bytes)); > DCHECK is probably sufficient. Done http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@240 PS5, Line 240: } : *slice = Slice(&pb_->indirect_data()[offset_in_indirect], ptr_slice->size()); : } else { > "After one row's column size has been found to exceed the limit and has bee Done http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@256 PS5, Line 256: } else { > DCHECK is probably good enough here too. Done http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@499 PS5, Line 499: // update to perform. > IIUC, we don't check cell sizes here because the primary key is immutable, Yes, otherwise we are not able to update or delete an exist, cell size exceed row. http://gerrit.cloudera.org:8080/#/c/13470/5/src/kudu/common/row_operations.cc@605 PS5, Line 605: RETURN_NOT_OK(op->split_row->Set(static_cast<int32_t>(tablet_col_idx), data)); > Why don't we check cell sizes here? Not check, because it's a split key, which will not stored in database. -- 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: 6 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: Mon, 29 Jul 2019 06:40:16 +0000 Gerrit-HasComments: Yes
