[kudu-CR] [common] add FindColumn for Schema
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13936 ) Change subject: [common] add FindColumn for Schema .. Patch Set 3: (3 comments) > Patch Set 3: > > Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/18395/ http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG@7 PS2, Line 7: Schema > Schema Done http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG@10 PS2, Line 10: now add it as a member of Schema. > Schema Done http://gerrit.cloudera.org:8080/#/c/13936/2/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/13936/2/src/kudu/common/schema.h@509 PS2, Line 509: // Find the column index corresponding to the given column name, > Doc. Done -- To view, visit http://gerrit.cloudera.org:8080/13936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iede12b95b774754f914295cecd2f6797008fed46 Gerrit-Change-Number: 13936 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Mon, 29 Jul 2019 05:53:08 + Gerrit-HasComments: Yes
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 ) Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc@582 PS8, Line 582: RETURN_NOT_OK(add_replica_func(resp.ts_infos(r.ts_info_idx()), r.role(), )); : } : : unique_ptr client_tablet(new KuduTablet); : client_tablet->data_ = new KuduTablet::Data(tablet_id, std::move(replicas)); : replicas.clear(); : > Seems like we could deduplicate this chunk of code too? Done http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc@198 PS8, Line 198: replicas.push_back(replica); > Hmm, guess I was wrong about std::move here and below. Emm, I think because RemoteReplica's members are pointers and enumerations, so there's no improvement in using std::move. Is that right? -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 9 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Mon, 29 Jul 2019 05:53:30 + Gerrit-HasComments: Yes
[kudu-CR] [common] add FindColumn for Schema
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13936 to look at the new patch set (#3). Change subject: [common] add FindColumn for Schema .. [common] add FindColumn for Schema There are several implementations of FindColumn, now add it as a member of Schema. Change-Id: Iede12b95b774754f914295cecd2f6797008fed46 --- M src/kudu/client/scan_batch.cc M src/kudu/common/partial_row.cc M src/kudu/common/row_operations-test.cc M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/master/master_service.cc 7 files changed, 49 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/13936/3 -- To view, visit http://gerrit.cloudera.org:8080/13936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iede12b95b774754f914295cecd2f6797008fed46 Gerrit-Change-Number: 13936 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13908 to look at the new patch set (#9). Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. [client] Deprecated TabletLocationsPB.ReplicaPB message in client In this patch, I removed the dependency on it from the client. It's only used for backward compatibility with RPC. In the future we will completely deprecated TabletLocationsPB.ReplicaPB message. Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/linked_list-test.cc M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/integration-tests/raft_config_change-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/rebalancer_tool-test.cc 27 files changed, 315 insertions(+), 236 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/9 -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 9 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu
[kudu-CR] [ksck] Filter tables and tablets in KsckCluster
Yifan Zhang has posted comments on this change. (
http://gerrit.cloudera.org:8080/13937 )
Change subject: [ksck] Filter tables and tablets in KsckCluster
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13937/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13937/2//COMMIT_MSG@9
PS2, Line 9: executes
> Nit: executes
Done
http://gerrit.cloudera.org:8080/#/c/13937/2/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/13937/2/src/kudu/tools/ksck-test.cc@201
PS2, Line 201: for (auto it = tables_.begin(); it != tables_.end();) {
: if (!MatchesAnyPattern(tabl
> for (auto& it = tables_.begin(); it != tables_.end(); ) {
Done
http://gerrit.cloudera.org:8080/#/c/13937/2/src/kudu/tools/ksck-test.cc@213
PS2, Line 213: sh
> '& '
Done
http://gerrit.cloudera.org:8080/#/c/13937/2/src/kudu/tools/ksck-test.cc@215
PS2, Line 215: if (!MatchesAnyPattern(tablet_id_filters_, (*it)->id())) {
: it = tablets.erase(it);
> for (auto& it = tablets.begin(); it != tablets.end(); ) {
Done
--
To view, visit http://gerrit.cloudera.org:8080/13937
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I23b6e6ef258d3498a42af7f92b63392a59c99761
Gerrit-Change-Number: 13937
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 03:52:05 +
Gerrit-HasComments: Yes
[kudu-CR] [ksck] Filter tables and tablets in KsckCluster
Hello Alexey Serbin, Yingchun Lai, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13937 to look at the new patch set (#3). Change subject: [ksck] Filter tables and tablets in KsckCluster .. [ksck] Filter tables and tablets in KsckCluster The ksck tool executes slowly if there are too many tables in a cluster, move table_name_filters and tablet_id_filters to KsckCluster and only check filtered cluster would speed up the execution of ksck. Change-Id: I23b6e6ef258d3498a42af7f92b63392a59c99761 --- M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_checksum.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/rebalancer.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_replica_util.cc 8 files changed, 76 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/13937/3 -- To view, visit http://gerrit.cloudera.org:8080/13937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I23b6e6ef258d3498a42af7f92b63392a59c99761 Gerrit-Change-Number: 13937 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] [tserver] Move cell and probe key size checking to prepare phase
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 <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Sun, 28 Jul 2019 22:28:19 +
Gerrit-HasComments: Yes
[kudu-CR] [ksck] Filter tables and tablets in KsckCluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13937 ) Change subject: [ksck] Filter tables and tablets in KsckCluster .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13937/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13937/2//COMMIT_MSG@9 PS2, Line 9: executed Nit: executes -- To view, visit http://gerrit.cloudera.org:8080/13937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23b6e6ef258d3498a42af7f92b63392a59c99761 Gerrit-Change-Number: 13937 Gerrit-PatchSet: 2 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Sun, 28 Jul 2019 22:07:36 + Gerrit-HasComments: Yes
[kudu-CR] [common] add FindColumn for Schecma
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13936 ) Change subject: [common] add FindColumn for Schecma .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG@7 PS2, Line 7: Schecma Schema http://gerrit.cloudera.org:8080/#/c/13936/2//COMMIT_MSG@10 PS2, Line 10: now add it as a member of Schecma. Schema http://gerrit.cloudera.org:8080/#/c/13936/2/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/13936/2/src/kudu/common/schema.h@509 PS2, Line 509: Status FindColumn(const Slice& col_name, int* idx) const; Doc. Also, you can pass col_name by value; Slices are small enough that it's probably more efficient to copy than to indirect. -- To view, visit http://gerrit.cloudera.org:8080/13936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iede12b95b774754f914295cecd2f6797008fed46 Gerrit-Change-Number: 13936 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 28 Jul 2019 22:01:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 ) Change subject: KUDU-2881 Support create/drop range partition by command line .. Patch Set 15: (6 comments) http://gerrit.cloudera.org:8080/#/c/13833/15//COMMIT_MSG Commit Message: PS15: Could you restore the commit message indentation as it was before? git will automatically indent the message after the commit is merged. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2350 PS15, Line 2350: Status InsertTestRows(const client::sp::shared_ptr& client, This function should be declared in an anonymous namespace, or declared static. It's also indented too much. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2372 PS15, Line 2372: RETURN_NOT_OK(session->Flush()); : RETURN_NOT_OK(session->Close()); This isn't necessary; what is being flushed here? http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2426 PS15, Line 2426: ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); ASSERT_OK(s) << ... would be more idiomatic. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2529 PS15, Line 2529: Status s = InsertTestRows(client_, kTestTableName, value, 1); : EXPECT_TRUE(s.IsNotFound()); You can combine this into one line. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc@575 PS15, Line 575: case KuduColumnSchema::BOOL: FALLTHROUGH_INTENDED; : case KuduColumnSchema::FLOAT: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DOUBLE: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DECIMAL: FALLTHROUGH_INTENDED; As I mentioned in my earlier comment here, for "direct chaining" (i.e. cases that don't execute a single line of code), you don't need to add FALLTHROUGH_INTENDED. It's only necessary when the case does something and then falls through. -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 15 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Sun, 28 Jul 2019 21:59:45 + Gerrit-HasComments: Yes
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 ) Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc@582 PS8, Line 582: // TODO(aserbin): try to use member_type instead of role for metacache. : bool is_leader = r.role() == consensus::RaftPeerPB::LEADER; : bool is_voter = is_leader || r.role() == consensus::RaftPeerPB::FOLLOWER; : unique_ptr replica(new KuduReplica); : replica->data_ = new KuduReplica::Data(is_leader, is_voter, std::move(ts)); : : replicas.push_back(replica.release()); Seems like we could deduplicate this chunk of code too? http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc@198 PS8, Line 198: replicas.push_back(std::move(replica)); > warning: std::move of the variable 'replica' of the trivially-copyable type Hmm, guess I was wrong about std::move here and below. -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 8 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Sun, 28 Jul 2019 21:50:51 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add a complete Nifi quickstart example
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13878 ) Change subject: [examples] Add a complete Nifi quickstart example .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71f3bc5898c15d7bc19cffb3a91b9efac3f6928b Gerrit-Change-Number: 13878 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 28 Jul 2019 21:49:56 + Gerrit-HasComments: No
[kudu-CR] [examples] Add a complete Nifi quickstart example
Grant Henke has removed a vote on this change. Change subject: [examples] Add a complete Nifi quickstart example .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I71f3bc5898c15d7bc19cffb3a91b9efac3f6928b Gerrit-Change-Number: 13878 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1938 Add non-copy setters to partial row pt 3
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13928 ) Change subject: KUDU-1938 Add non-copy setters to partial row pt 3 .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13928/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13928/3//COMMIT_MSG@14 PS3, Line 14: to already be truncated (which it is in Impala's case) and only check > is this a safe assumption? last I was aware, Impala's treatment of "string" More context on bytes vs. characters (from part 1 of this series): https://gerrit.cloudera.org/c/13760/20/src/kudu/common/schema.h#126). -- To view, visit http://gerrit.cloudera.org:8080/13928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f2aba098d649eb94e0314f6606cc33600e8d766 Gerrit-Change-Number: 13928 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sun, 28 Jul 2019 21:43:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#15). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 768 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/15 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 15 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#14). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 773 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/14 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 14 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
honeyhexin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
..
Patch Set 13:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2357
PS12, Line 2357: for (int i = start_key; i < num_rows + start_key; ++i) {
: unique_ptr insert(table->NewInsert());
: auto* row = insert->mutable_row();
: RETURN_NOT_OK(row->SetInt32("key", i));
: RETURN_NOT_OK(row->SetInt32("int_val", 12345));
: RETURN_NOT_OK(row->SetString("string_val", "hello"));
: Status result = session->Apply(insert.release());
: if (result.IsIOError()) {
: vector errors;
: ElementDeleter drop();
: bool overflowed;
: session->GetPendingErrors(, );
: EXPECT_FALSE(overflowed);
: EXPECT_EQ(1, errors.size());
:
> This seems pretty similar to insert_test_rows(), defined in the next test.
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2401
PS12, Line 2401: // drop all rows when dropping range partition. After
dropping there will
: // be 0 rows left.
: string stdout, stderr;
: Status s = RunKuduTool({
: "table",
: "drop_range_partition",
: master_addr,
: kTableId,
> ASSERT_OK() here?
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2461
PS12, Line 2461: ASSERT_OK(table_creator->table_name(kTestTableName)
> How about we catch any errors here, and "unwrap" the pending errors and ret
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2668
PS12, Line 2668:
: TEST_F(AdminCliTest,
TestAddAndDropRangePartitionWithWrongParameters) {
: FLAGS_num_tablet_servers = 1;
: FLAGS_num_replicas = 1;
:
: NO_FATALS(BuildAndStart());
:
: const string& master_addr =
cluster_->master()->bound_rpc_addr().ToString();
: const string kTestTableName = "TestTable1";
:
: // Build the schema.
: KuduSchema schema;
: KuduSchemaBuilder builder;
:
builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull();
: builder.SetPrimaryKey({ "key" });
: ASSERT_OK(builder.Build());
:
: unique_ptr lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 0));
: unique_ptr upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 1));
:
: unique_ptr
table_creator(client_->NewTableCreator());
: ASSERT_OK(table_creator->table_name(kTestTableName)
:
> This same pattern is repeated many times. Maybe wrap this in a lambda like:
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2696
PS12, Line 2696: .Create());
:
: // Lambda function to c
> Is there a reason we're using 1000 rows? Why not 100 like in the other test
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2790
PS12, Line 2790: ASSERT_OK(lower_bound->SetInt32("key_INT32", 2));
: ASSERT_OK(lower_bou
> nit: could we pass in just the argument, and have this be:
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@98
PS12, Line 98: upper
> upper
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@484
PS12, Line 484: Status ConvertToKuduPartialRow(const
client::sp::shared_ptr& table,
> Could some of TableScanner be reused for this? There's logic there to deser
As we have discussed offline , keep this logic here.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@592
PS12, Line 592:
: Status ModifyRangePartition(const RunnerContext& context,
PartitionAction action) {
: const string& table_name = FindOrDie(context.required_args,
kTableNameArg);
: const string& table_range_lower_bound =
FindOrDie(context.required_args,
:
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#13). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 771 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/13 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 13 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
