[kudu-CR] KUDU-1945 Auto-Incrementing Column

["Abhishek Chennaka (Code Review)" <ge...@cloudera.org>]

Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )

Change subject: KUDU-1945 Auto-Incrementing Column
......................................................................


Patch Set 7:

(10 comments)

Will create a new test case under integrated_tests to scan all the replicas in future revisions of this patch.

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc@9866
PS7, Line 9866:     ASSERT_OK(ScanToStrings(&scanner, &rows));
> Does it make sense to add an assert on rows.size() == kNumRows ?
Done


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/client-test.cc@9882
PS7, Line 9882:     ASSERT_OK(ScanToStrings(&scanner, &rows));
> Does it make sense to add an assert on rows.size() == kNumRows ?
Done


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.h@223
PS7, Line 223:     UINT64 = 4,
             :     STRING = 5,
             :     BOOL = 6,
             :     FLOAT = 7,
             :     DOUBLE = 8,
             :     BINARY = 9,
             :     UNIXTIME_MICROS = 10,
             :     DECIMAL = 11,
             :     VARCHAR = 12,
             :     TIMESTAMP = UNIXTIME_MICROS, //!< deprecated, use UNIXTIME_MICROS
             :     DATE = 13
> A couple of things here:
Thanks for pointing that out. Posted a new patch for this.


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/client/schema.cc@477
PS7, Line 477: data_->auto_incrementing
> Should we also check internal_type and nullable if data_->auto_incrementing
Done


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

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@495
PS7, Line 495: INT64_MAX
> I guess this value is different now since switching to uint64.
Thanks for the catch, fixed it.


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@496
PS7, Line 496: Status::InvalidArgument("max auto-incrementing value reached")
> Status::IllegalState() seems to fit better here unless there is required to
We should not be allowing overflow of the counter and the reason being - if a user upserts UINT64_MAX number of rows, the next upserted row with the same primary key would update the existing row and not create a new row. 
The user expects UINT64_MAX+1 rows to be present but there are only UINT64_MAX rows present.
So the tablet should not be receiving any writes. We can probably have an unsafe CLI tool to reset this counter in a separate changelist.


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/row_operations.cc@497
PS7, Line 497:           // TODO(achennaka): return?
             :         }
             :         (*auto_incrementing_counter)++;
             :         memcpy(tablet_row.mutable_cell_ptr(tablet_col_idx), auto_incrementing_counter, 8);
             :         BitmapChange(tablet_isset_bitmap, client_col_idx, true);
> Well, at this this shouldn't be done if interpreting maximum possible value
True, I think it makes sense to return in line 497.


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

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/common/schema.h@241
PS7, Line 241:                const void* write_default = nullptr,
> BTW, do we allow setting write_default for an auto-incrementing column?  I 
Added the verification in schema.cc


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tablet/ops/write_op.cc@160
PS7, Line 160: GetAutoIncrementingCounter() != 0
> What if counter starts with 0 or overflown and its current value is 0?
Right, fixed the logic.


http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

http://gerrit.cloudera.org:8080/#/c/19097/7/src/kudu/tserver/tserver.proto@142
PS7, Line 142: required
> To allow for easier migration to proto3 consider using 'optional' here inst
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/19097
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dbde9095da78f6d1bd00adcc0a6e7dd63082bbc
Gerrit-Change-Number: 19097
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Dec 2022 06:54:39 +0000
Gerrit-HasComments: Yes