You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/05/30 07:57:32 UTC

[kudu-CR] [tserver] Move cell size checking to prepare phase 

Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13470


Change subject: [tserver] Move cell size checking to prepare phase
......................................................................

[tserver] Move cell size checking to prepare phase

It's inefficiency to check cell size in apply phase to consult every
columns of rows, this patch move this checking to prepare phase when
decoding values from protobuf.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/tablet.cc
M src/kudu/util/safe_math.h
5 files changed, 229 insertions(+), 78 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/1
-- 
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: newchange
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficiency to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move these light weight checking to prepare phase when
decoding values from protobuf and before lock row key.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 253 insertions(+), 86 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/4
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG@9
PS2, Line 9: It's inefficiency to check cell size in apply phase to consult every
           : columns of rows
> Why is it more efficient to perform this check during prepare? Can you quan
In prepare phase, we can check cell size when decode data from client request, then short circuit return, without going to apply phase. However, checking cell size in apply phase, we have to traverse each row and each cell again, and construct many Slice for BINARY type.


http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc@545
PS2, Line 545:   if (PREDICT_FALSE(enc_key_size > FLAGS_max_encoded_key_size_bytes)) {
             :     return Status::InvalidArgument(Substitute(
             :         "encoded primary key too large ($0 bytes, maximum is $1 bytes)",
             :         enc_key_size, FLAGS_max_encoded_key_size_bytes));
             :   }
> Could/should we move this too? Looks like op.key_probe is created when row 
I think we should, return error as early as possible is reasonable.



-- 
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 09:23:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sun, 28 Jul 2019 22:28:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+1


-- 
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: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 04:22:17 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 06:40:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 240 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/8
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................

[tserver] Move cell size checking to prepare phase

It's inefficiency to check cell size in apply phase to consult every
columns of rows, this patch move this checking to prepare phase when
decoding values from protobuf.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/safe_math.h
6 files changed, 292 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/2
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.h@107
PS6, Line 107:   // Read one row's column data from 'src_', read result is stored in 'slice'.
             :   // Return bad Status if data is corrupt.
             :   // NOTE: If column data validate error (i.e. column size exceed the limit), only
             :   // set bad Status to 'row_status', and returm Status::OK.
Should also add how setting row_status to nullptr or not will affect column size checking.


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@605
PS5, Line 605:       RETURN_NOT_OK(op->split_row->Set(static_cast<int32_t>(tablet_col_idx), data));
> Not check, because it's a split key, which will not stored in database.
Right, and a split key will get validated when, after it's been serialized into a PartitionPB and stored in a SysTabletsEntryPB, the master tablet writes the SysTabletsEntryPB. At that point, the entire SysTabletsEntryPB is written out as a STRING and will get checked via the standard INSERT or UPDATE code paths.


http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc@421
PS6, Line 421:         if (!row_status.ok()) op->SetFailureStatusOnce(row_status);
Could PREDICT_FALSE here.


http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc@518
PS6, Line 518:           if (!row_status.ok()) op->SetFailureStatusOnce(row_status);
Could PREDICT_FALSE 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: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 18:37:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG@9
PS2, Line 9: It's inefficiency to check cell size in apply phase to consult every
           : columns of rows
> In prepare phase, we can check cell size when decode data from client reque
On the other hand, the prepare phase is single-threaded, whereas the apply phase is multithreaded. So, as much work as we can keep off of 'prepare' the better, IMO.

Also I think changing semantics to reject the whole request here if one row is bad is likely to break applications.



-- 
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 16:44:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 3:

> Patch Set 2:
> 
> (1 comment)

Size checking is light enough, and row-key will not lock when row error checked, I think it's worth doing that.


-- 
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: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 27 Jul 2019 10:34:27 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 240 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/10
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG@9
PS2, Line 9: It's inefficiency to check cell and probe key size in apply phase,
           : because we have
> On the other hand, the prepare phase is single-threaded, whereas the apply 
Done


http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc@545
PS2, Line 545:   }
             :   __builtin_unreachable();
             : }
             : 
             : Sta
> I think we should, return error as early as possible is reasonable.
Done



-- 
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: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 27 Jul 2019 16:10:50 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 2:

Does this change user-facing behavior? Previously, if there were a too-large cell in a batch with other valid rows, would the other rows succeed (and the error get reported as a per-row error?)


-- 
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 30 May 2019 19:08:36 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+1

(1 comment)

Todd, do you want to take a look too?

http://gerrit.cloudera.org:8080/#/c/13470/7/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/13470/7/src/kudu/common/row_operations.h@111
PS7, Line 111: returm
return



-- 
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: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 04:04:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficiency to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move these light weight checking to prepare phase when
decoding values from protobuf and before lock row key.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 253 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/5
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
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: deleteReviewer
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 239 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/6
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> Does this change user-facing behavior? Previously, if there were a too-large cell in a batch with other valid rows, would the other rows succeed (and the error get reported as a per-row error?)

Yes, the whole operation will fail if any row's cell size is too large.


-- 
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 08:50:51 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 9:

(1 comment)

> Uploaded patch set 9.

Just rebase master to solve conflict.

http://gerrit.cloudera.org:8080/#/c/13470/7/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/13470/7/src/kudu/common/row_operations.h@111
PS7, Line 111: return
> return
Done



-- 
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: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 31 Jul 2019 06:55:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.h@107
PS6, Line 107:   // Read one row's column data from 'src_', read result is stored in 'slice'.
             :   // Return bad Status if data is corrupt.
             :   // NOTE: If 'row_status' is not nullptr, column data validate will be performed,
             :   // and if column data validate error (i.e. column size ex
> Should also add how setting row_status to nullptr or not will affect column
Done


http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:

http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc@421
PS6, Line 421:         if (PREDICT_FALSE(!row_status.ok())) op->SetFailureStatusOnce(row_status);
> Could PREDICT_FALSE here.
Done


http://gerrit.cloudera.org:8080/#/c/13470/6/src/kudu/common/row_operations.cc@518
PS6, Line 518:           if (PREDICT_FALSE(!row_status.ok())) op->SetFailureStatusOnce(row_status);
> Could PREDICT_FALSE here.
Done



-- 
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: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 03:42:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 10: Verified+1 Code-Review+1

Overriding Jenkins, known flake.


-- 
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: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 03:04:00 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 240 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/7
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13470/2//COMMIT_MSG@9
PS2, Line 9: It's inefficiency to check cell size in apply phase to consult every
           : columns of rows
Why is it more efficient to perform this check during prepare? Can you quantify this?


http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

http://gerrit.cloudera.org:8080/#/c/13470/2/src/kudu/tablet/tablet.cc@545
PS2, Line 545:   if (PREDICT_FALSE(enc_key_size > FLAGS_max_encoded_key_size_bytes)) {
             :     return Status::InvalidArgument(Substitute(
             :         "encoded primary key too large ($0 bytes, maximum is $1 bytes)",
             :         enc_key_size, FLAGS_max_encoded_key_size_bytes));
             :   }
Could/should we move this too? Looks like op.key_probe is created when row locks are taken, which is also during prepare, but after the operations themselves are decoded.



-- 
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 30 May 2019 23:40:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Reviewed-on: http://gerrit.cloudera.org:8080/13470
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 240 insertions(+), 87 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, but someone else must approve; Verified
  Todd Lipcon: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13470 )

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................


Patch Set 3:

> Patch Set 2:
> 
> Does this change user-facing behavior? Previously, if there were a too-large cell in a batch with other valid rows, would the other rows succeed (and the error get reported as a per-row error?)

Per-row error in prepare phase is supported by a prior patch, now we can record an error when cell size exceed in prepare phase, user-facing behavior is not changed.


-- 
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: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 27 Jul 2019 10:31:13 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell and probe key size checking to prepare phase
......................................................................

[tserver] Move cell and probe key size checking to prepare phase

It's inefficient to check cell and probe key size in apply phase,
because we have to lock row key and construct new Slices for every
binary type of columns.
This patch move this lightweight checking to prepare phase when
decoding values from protobuf and before locking row keys.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/clock/clock.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
9 files changed, 242 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/9
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>

[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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 10: Code-Review+2

Took a quick look and looks OK to me.


-- 
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: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 12 Aug 2019 18:16:59 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] Move cell size checking to prepare phase

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: [tserver] Move cell size checking to prepare phase
......................................................................

[tserver] Move cell size checking to prepare phase

It's inefficiency to check cell size in apply phase to consult every
columns of rows, this patch move this checking to prepare phase when
decoding values from protobuf.

Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/tablet.cc
4 files changed, 219 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/13470/3
-- 
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: newpatchset
Gerrit-Change-Id: Id3e672272bb1dcf2d0ac1d96ee8a1a2d1489774c
Gerrit-Change-Number: 13470
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>