Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18663 )
Change subject: WIP [c++ client] KUDU-2671 Custom hash schema alter table support ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.h@1896 PS2, Line 1896: rows > What rows? It was a copy paste error. Fixed it now. http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.h@1959 PS2, Line 1959: rows > ? same as above http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.h@1960 PS2, Line 1960: /// : /// @note Multiple range partitions may be added as part of a single alter : /// table transaction by calling this method multiple times on the table : /// alterer. : /// : /// @note This client may immediately write and scan the new tablets when : /// Alter() returns success, however other existing clients may have to wait : /// for a timeout period to elapse before the tablets become visible. This : /// period is configured by the master's 'table_locations_ttl_ms' flag, and : /// defaults to 5 minutes. : /// : /// @param [in] partition : /// The Kudu Range partition to be created. This Kudu Range partition can : /// have a custom hash schema defined. : /// @param [in] dimension_label : /// The dimension label for the tablet to be created. : /// @return Raw pointer to this alterer object. > nite: instead of copy-pasting this, consider using the @copydoc directive: Thanks, as discussed, appears this would be useful if the parameters are identical or a subset between the methods. http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.h@1979 PS2, Line 1979: const std::string& dimension_label); > I'm not sure we want to support dimension labels for this right out of the Makes sense. Got rid of the dimension labels support as it can be added later if needed. http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.cc@1504 PS2, Line 1504: > nit: an extra line Done http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/client.cc@1602 PS2, Line 1602: KuduTableCreator::KuduRangePartition* range_partition(new KuduTableCreator::KuduRangePartition( : lower_bound, upper_bound, lower_bound_type, upper_bound_type)); > Is this ever used? No, it's not supposed to be there. Fixed it now. http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.h@62 PS2, Line 62: Kudu Range partition > nit: range partition Done http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.cc@170 PS2, Line 170: > nit: wrong indent -- should be 4 spaces from the begining of the previous l Done http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.cc@175 PS2, Line 175: s.range_partition->data_ > nit: you could introduce a pointer variable for s.range_partition->data_ an Done http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.cc@182 PS2, Line 182: if (!s.range_partition->data_->hash_schema_.empty()) { > nit: this condition isn't necessary since the cycle below iterates over s.r Good catch. Fixed it. http://gerrit.cloudera.org:8080/#/c/18663/2/src/kudu/client/table_alterer-internal.cc@183 PS2, Line 183: hash_schema > nit: rename into 'hash_dimension' to keep the terminology consistent across Done -- To view, visit http://gerrit.cloudera.org:8080/18663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4b1e306cca096d9479f06669cc22cc40d77fb42 Gerrit-Change-Number: 18663 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 29 Jun 2022 19:43:24 +0000 Gerrit-HasComments: Yes
