[kudu-CR] [tools] added insert-generated-rows into kudu tools
Alexey Serbin has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 16: (3 comments) http://gerrit.cloudera.org:8080/#/c/4412/16/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 180: TEST_F(ToolTest, TestTopLevelHelp) { > Can you also add a new section to TestModeHelp for the new action? Done PS16, Line 788: vector( > Sorry, I meant why do you need a vector() wrapper? Ah, sorry for the misunderstanding. That's not needed: good catch! http://gerrit.cloudera.org:8080/#/c/4412/17/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: PS17, Line 79: using std::unique_ptr; : using std::vector; > Nit: back_inserter should go first. Sounds like a pun :) Fixed. -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4412 to look at the new patch set (#18). Change subject: [tools] added insert-generated-rows into kudu tools .. [tools] added insert-generated-rows into kudu tools The insert-generated-rows tool has been merged into the 'kudu' umbrella toolset. This addresses KUDU-1628. Besides, added ability to run multiple inserter threads and specify additional parameters on batching behavior of the generated write operations. It's possible to run data generating sessions both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes. Introduced sequential and random modes for the data generator. Overall, these changes allow to use the tool to measure performance of the Kudu C++ client library in simplistic 'push-as-much-as-you-can' scenario: the client generates and sends data as fast as it can. Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad --- M src/kudu/tools/CMakeLists.txt D src/kudu/tools/insert-generated-rows.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/tools/tool_action_remote_replica.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc A src/kudu/tools/tool_action_test.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_action_wal.cc M src/kudu/tools/tool_main.cc 17 files changed, 714 insertions(+), 183 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/18 -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Adar Dembo has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 17: (3 comments) http://gerrit.cloudera.org:8080/#/c/4412/16/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 180: string tool_path_; > Done Can you also add a new section to TestModeHelp for the new action? PS16, Line 788: ema(client::Kud > Because otherwise it will be an error like Sorry, I meant why do you need a vector() wrapper? http://gerrit.cloudera.org:8080/#/c/4412/17/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: PS17, Line 79: using std::copy; : using std::back_inserter; Nit: back_inserter should go first. -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
Adar Dembo has posted comments on this change. Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds .. Patch Set 2: Verified+1 Overriding Jenkins, flakiness in ITClient. -- To view, visit http://gerrit.cloudera.org:8080/4511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Alexey Serbin has posted comments on this change. Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Alexey Serbin has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/4412/16/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 180: TEST_F(ToolTest, TestTopLevelHelp) { > Can you update this test with the new mode and action? Done PS16, Line 781: client::sp:: > Nit: you added a "using" for this at the top of the file. Done PS16, Line 788: vector( > Why is this needed? Because otherwise it will be an error like Bad status: Invalid argument: Table partitioning must be specified using add_hash_partitions or set_range_partition_columns PS16, Line 789: wait(true) > Can omit, this is the default behavior. Done PS16, Line 798: ASSERT_OK(cluster->Start()); > You can move this into LoadgenCreateMiniCluster if you want. You could even Good idea. For some reason, I thought keeping separate functions would be better, but apparently there isn't any demand for that. Will update. PS16, Line 819: string("-table_name=") + kTableName > Nit: Substitute("-table_name=%s", kTableName) is more idiomatic. Done Line 820: "-buffer_flush_watermark_pct=0.125", > Nit: can you use -- to prefix gflags, for consistency with existing tests? Done -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4412 to look at the new patch set (#17). Change subject: [tools] added insert-generated-rows into kudu tools .. [tools] added insert-generated-rows into kudu tools The insert-generated-rows tool has been merged into the 'kudu' umbrella toolset. This addresses KUDU-1628. Besides, added ability to run multiple inserter threads and specify additional parameters on batching behavior of the generated write operations. It's possible to run data generating sessions both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes. Introduced sequential and random modes for the data generator. Overall, these changes allow to use the tool to measure performance of the Kudu C++ client library in simplistic 'push-as-much-as-you-can' scenario: the client generates and sends data as fast as it can. Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad --- M src/kudu/tools/CMakeLists.txt D src/kudu/tools/insert-generated-rows.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/tools/tool_action_remote_replica.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc A src/kudu/tools/tool_action_test.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_action_wal.cc M src/kudu/tools/tool_main.cc 17 files changed, 708 insertions(+), 182 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/17 -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: fix up libtool scripts if needed
Dan Burkert has posted comments on this change. Change subject: thirdparty: fix up libtool scripts if needed .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1654 - [python] Python 3 Client Test Failure: test table column
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4543 to look at the new patch set (#3). Change subject: KUDU-1654 - [python] Python 3 Client Test Failure: test_table_column .. KUDU-1654 - [python] Python 3 Client Test Failure: test_table_column Python 3 fails the test_table_column test because it is is attempting to cast a string to bytes without an encoding argument. In Python 3, this is a required parameter as bytes and strings are no longer synonymous. This patch utilizes the kudu.compat.frombytes method which was written for this purpose. Change-Id: I7184639426b7f2528523209152dafd5fa39fecf9 --- M python/kudu/client.pyx M python/kudu/tests/test_client.py 2 files changed, 7 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/4543/3 -- To view, visit http://gerrit.cloudera.org:8080/4543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7184639426b7f2528523209152dafd5fa39fecf9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1654 - [python] Python 3 Client Test Failure: test table column
Jordan Birdsell has posted comments on this change. Change subject: KUDU-1654 - [python] Python 3 Client Test Failure: test_table_column .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4543/1/python/kudu/tests/test_client.py File python/kudu/tests/test_client.py: Line 45: assert col.name == name > given that column names should typically be human-readable (and are Strings Done -- To view, visit http://gerrit.cloudera.org:8080/4543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7184639426b7f2528523209152dafd5fa39fecf9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1654 - [python] Python 3 Client Test Failure: test table column
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4543 to look at the new patch set (#2). Change subject: KUDU-1654 - [python] Python 3 Client Test Failure: test_table_column .. KUDU-1654 - [python] Python 3 Client Test Failure: test_table_column Python 3 fails the test_table_column test because it is is attempting to cast a string to bytes without an encoding argument. In Python 3, this is a required parameter as bytes and strings are no longer synonymous. This patch utilizes the kudu.compat.frombytes method which was written for this purpose. Change-Id: I7184639426b7f2528523209152dafd5fa39fecf9 --- M python/kudu/client.pyx M python/kudu/tests/test_client.py 2 files changed, 8 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/4543/2 -- To view, visit http://gerrit.cloudera.org:8080/4543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7184639426b7f2528523209152dafd5fa39fecf9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Adar Dembo has posted comments on this change. Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Jean-Daniel Cryans has posted comments on this change. Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4560/1/docs/release_notes.adoc File docs/release_notes.adoc: PS1, Line 50: emphasis > You missed "emphasize" for "emphasis". oops -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4560 to look at the new patch set (#4). Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. [docs] Release note for OperationResponse.getWriteTimestamp See https://gerrit.cloudera.org/#/c/4487/ Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 --- M docs/release_notes.adoc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/4560/4 -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] thirdparty: fix up libtool scripts if needed
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4512 to look at the new patch set (#4). Change subject: thirdparty: fix up libtool scripts if needed .. thirdparty: fix up libtool scripts if needed Older versions of libtool (such as the version found on el6) do not pass -stdlib=libc++ found in CXXFLAGS down to the libtool link command line, and as a result, the shared object is linked against the system libstdc++. The net effect is that some thirdparty shared objects sprout @@GLIBCXX dependencies. Mysteriously, Kudu libraries and binaries themselves do not, and the @@GLIBCXX dependencies appear to be satisfied via libc++ somehow. Nevertheless, this is confusing and could prove problematic down the road, so here's a quick hack to "fix up" libtool scripts after they are generated via configure. Change-Id: Id51c10d38984e892496621135634d21f3e2386ce --- M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh A thirdparty/postflight.py 3 files changed, 136 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4512/4 -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Adar Dembo has posted comments on this change. Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4560/1/docs/release_notes.adoc File docs/release_notes.adoc: PS1, Line 50: emphasis > Done You missed "emphasize" for "emphasis". -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Jean-Daniel Cryans has posted comments on this change. Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4560/1/docs/release_notes.adoc File docs/release_notes.adoc: PS1, Line 49: renamed > Nit: renamed to Done PS1, Line 50: emphasis > Nit: emphasize that it doesn't return milliseconds. Done -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4560 to look at the new patch set (#3). Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. [docs] Release note for OperationResponse.getWriteTimestamp See https://gerrit.cloudera.org/#/c/4487/ Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 --- M docs/release_notes.adoc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/4560/3 -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Jean-Daniel Cryans has uploaded a new patch set (#2). Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. [docs] Release note for OperationResponse.getWriteTimestamp See https://gerrit.cloudera.org/#/c/4487/ Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 --- M docs/release_notes.adoc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/4560/2 -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Adar Dembo has posted comments on this change. Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4560/1/docs/release_notes.adoc File docs/release_notes.adoc: PS1, Line 49: renamed Nit: renamed to PS1, Line 50: emphasis Nit: emphasize that it doesn't return milliseconds. -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [WIP]KUDU-1640 - [python] Add IN-list predicate support
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4548 to look at the new patch set (#3). Change subject: [WIP]KUDU-1640 - [python] Add IN-list predicate support .. [WIP]KUDU-1640 - [python] Add IN-list predicate support This patch adds IN list predicate support for the python client. This patch includes a test. Change-Id: I932dfded62e162cf85e0e12432cf6716311957de --- M python/kudu/client.pyx M python/kudu/libkudu_client.pxd M python/kudu/tests/test_scanner.py 3 files changed, 27 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/4548/3 -- To view, visit http://gerrit.cloudera.org:8080/4548 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp
Jean-Daniel Cryans has uploaded a new change for review. http://gerrit.cloudera.org:8080/4560 Change subject: [docs] Release note for OperationResponse.getWriteTimestamp .. [docs] Release note for OperationResponse.getWriteTimestamp See https://gerrit.cloudera.org/#/c/4487/ Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 --- M docs/release_notes.adoc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/4560/1 -- To view, visit http://gerrit.cloudera.org:8080/4560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans
[kudu-CR] KUDU-1612 - [python] Enable setting of read mode for scanning
Jordan Birdsell has posted comments on this change. Change subject: KUDU-1612 - [python] Enable setting of read mode for scanning .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4520/2/python/kudu/tests/util.py File python/kudu/tests/util.py: Line 122: self.snapshot_timestamp = self.client.latest_observed_timestamp() > because the server's clock might not have the same time as the system's clo I follow, done. I took an approach that converts hybridtime to datetimes (via micros) to keep it a bit cleaner on the interface side if someone wanted to do anything with the latest observed timestamp value. -- To view, visit http://gerrit.cloudera.org:8080/4520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c61ef09f6e15bad2c44d9caf85b2cc2582b8a49 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1612 - [python] Enable setting of read mode for scanning
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4520 to look at the new patch set (#3). Change subject: KUDU-1612 - [python] Enable setting of read mode for scanning .. KUDU-1612 - [python] Enable setting of read mode for scanning Currently the python client is unable to set the read mode for scanning, so all scans are done as READ_LATEST. This patch enables the ability to set the read mode so that the python client can read at snapshots. This patch includes multiple tests. Change-Id: I2c61ef09f6e15bad2c44d9caf85b2cc2582b8a49 --- M python/kudu/__init__.py M python/kudu/client.pyx M python/kudu/libkudu_client.pxd M python/kudu/tests/test_scanner.py M python/kudu/tests/test_scantoken.py M python/kudu/tests/util.py M python/kudu/util.py 7 files changed, 275 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/4520/3 -- To view, visit http://gerrit.cloudera.org:8080/4520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2c61ef09f6e15bad2c44d9caf85b2cc2582b8a49 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
Dan Burkert has posted comments on this change. Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] c++ client: stop requiring the old gcc ABI
Dan Burkert has posted comments on this change. Change subject: c++ client: stop requiring the old gcc ABI .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: fix up libtool scripts if needed
Dan Burkert has posted comments on this change. Change subject: thirdparty: fix up libtool scripts if needed .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: build C dependencies with TSAN instrumentation too
Dan Burkert has posted comments on this change. Change subject: thirdparty: build C dependencies with TSAN instrumentation too .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4559 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie8274ff7bc57016733cdd3c26858483f74e68faf Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Alexey Serbin has posted comments on this change. Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/4526/6/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: PS6, Line 191: this > This lambda function is accessing ConsensusStatePBToHtml method. Not the ri Ah, I see -- I didn't spot this during the review. That's OK. -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Dan Burkert has posted comments on this change. Change subject: thirdparty: stifle unused argument warnings when building with clang .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: split into dependency groups
Dan Burkert has posted comments on this change. Change subject: thirdparty: split into dependency groups .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: reorganize tree
Dan Burkert has posted comments on this change. Change subject: thirdparty: reorganize tree .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java-client] IN-list predicate
Dan Burkert has submitted this change and it was merged. Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Reviewed-on: http://gerrit.cloudera.org:8080/4530 Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 616 insertions(+), 118 deletions(-) Approvals: Jean-Daniel Cryans: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1363: Add IN-list predicate type
Dan Burkert has submitted this change and it was merged. Change subject: KUDU-1363: Add IN-list predicate type .. KUDU-1363: Add IN-list predicate type Adds support in the C++ client for providing a set of equalities for a given column. Support for using IN list predicates from the Java client will be in a follow-up commit. Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Reviewed-on: http://gerrit.cloudera.org:8080/2986 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/predicate-test.cc M src/kudu/client/scan_predicate-internal.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/column_predicate.h M src/kudu/common/common.proto M src/kudu/common/key_util.cc M src/kudu/common/scan_spec-test.cc M src/kudu/common/scan_spec.cc M src/kudu/common/wire_protocol-test.cc M src/kudu/common/wire_protocol.cc 16 files changed, 1,000 insertions(+), 37 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: build C dependencies with TSAN instrumentation too
Hello Dan Burkert, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4559 to review the following change. Change subject: thirdparty: build C dependencies with TSAN instrumentation too .. thirdparty: build C dependencies with TSAN instrumentation too Our C library dependencies are pretty small so this isn't a huge hit, and it makes the sanitizer-based dependency stack much more consistent. Now, 'common' is only for tools and header-only dependencies, while uninstrumented/tsan/... include every library. With additional sanitization come new suppressions. I didn't spend a whole lot of time trying to figure out which were legitimate. Change-Id: Ie8274ff7bc57016733cdd3c26858483f74e68faf --- M build-support/tsan-suppressions.txt M cmake_modules/FindPmem.cmake M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh 4 files changed, 144 insertions(+), 121 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/4559/1 -- To view, visit http://gerrit.cloudera.org:8080/4559 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie8274ff7bc57016733cdd3c26858483f74e68faf Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Adar Dembo has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/4412/16/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 180: TEST_F(ToolTest, TestTopLevelHelp) { Can you update this test with the new mode and action? PS16, Line 781: client::sp:: Nit: you added a "using" for this at the top of the file. PS16, Line 788: vector( Why is this needed? PS16, Line 789: wait(true) Can omit, this is the default behavior. PS16, Line 798: ASSERT_OK(cluster->Start()); You can move this into LoadgenCreateMiniCluster if you want. You could even move LoadgenCreateTable() (enabled via an argument) and Subprocess::Call(), then call it RunLoadgenTest() or something. Basically make the TEST_F() functions themselves just thin wrappers that provide some additional arguments. PS16, Line 819: string("-table_name=") + kTableName Nit: Substitute("-table_name=%s", kTableName) is more idiomatic. Line 820: "-buffer_flush_watermark_pct=0.125", Nit: can you use -- to prefix gflags, for consistency with existing tests? -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4412 to look at the new patch set (#16). Change subject: [tools] added insert-generated-rows into kudu tools .. [tools] added insert-generated-rows into kudu tools The insert-generated-rows tool has been merged into the 'kudu' umbrella toolset. This addresses KUDU-1628. Besides, added ability to run multiple inserter threads and specify additional parameters on batching behavior of the generated write operations. It's possible to run data generating sessions both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes. Introduced sequential and random modes for the data generator. Overall, these changes allow to use the tool to measure performance of the Kudu C++ client library in simplistic 'push-as-much-as-you-can' scenario: the client generates and sends data as fast as it can. Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad --- M src/kudu/tools/CMakeLists.txt D src/kudu/tools/insert-generated-rows.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/tools/tool_action_remote_replica.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc A src/kudu/tools/tool_action_test.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_action_wal.cc M src/kudu/tools/tool_main.cc 17 files changed, 720 insertions(+), 179 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/16 -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: split into dependency groups
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4513 to look at the new patch set (#3). Change subject: thirdparty: split into dependency groups .. thirdparty: split into dependency groups The monolithic thirdparty build is now quite a bit larger than it used to be on account of the extra LLVM build. Let's see if we can't speed it up. The idea is simple: carve it up into disjoint sections so that individual sections can be rebuilt as needed. This patch separates the various portions of the thirdparty build into "dependency groups". Conceptually, a dependency group is a set of dependencies built a certain way, but the implementation is really just a set of non-overlapping code fragments in build-thirdparty.sh. The initial set of groups are: - common: dependencies that are never instrumented. - uninstrumented: dependencies that may be instrumented, but aren't in this build. - tsan: dependencies that may be instrumented, and are indeed in this build (with -fsanitize=thread). These three generally map to the existing "common", "uninstrumented", and "tsan" thirdparty subdirectories. There's an obvious pattern here for future sanitizer builds (e.g. MSAN would provide an "msan" dependency group). The new build-if-necessary.sh can accept an argument that maps to a set of dependency groups representing a particular build. Every dependency group has its own hash/stamp file so that it is only rebuilt when needed. This also fixes a bug in the stamp file approach that prevented it from actually rebuilding anything. Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 --- M CMakeLists.txt M build-support/jenkins/build-and-test.sh M thirdparty/.gitignore M thirdparty/build-if-necessary.sh M thirdparty/build-thirdparty.sh 5 files changed, 271 insertions(+), 202 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4513/3 -- To view, visit http://gerrit.cloudera.org:8080/4513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: fix up libtool scripts if needed
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4512 to look at the new patch set (#3). Change subject: thirdparty: fix up libtool scripts if needed .. thirdparty: fix up libtool scripts if needed Older versions of libtool (such as the version found on el6) do not pass -stdlib=libc++ found in CXXFLAGS down to the libtool link command line, and as a result, the shared object is linked against the system libstdc++. The net effect is that some thirdparty shared objects sprout @@GLIBCXX dependencies. Mysteriously, Kudu libraries and binaries themselves do not, and the @@GLIBCXX dependencies appear to be satisfied via libc++ somehow. Nevertheless, this is confusing and could prove problematic down the road, so here's a quick hack to "fix up" libtool scripts after they are generated via configure. Change-Id: Id51c10d38984e892496621135634d21f3e2386ce --- M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh A thirdparty/postflight.py 3 files changed, 131 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4512/3 -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1363: Add IN-list predicate type
Alexey Serbin has posted comments on this change. Change subject: KUDU-1363: Add IN-list predicate type .. Patch Set 19: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
Adar Dembo has posted comments on this change. Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4511/2/CMakeLists.txt File CMakeLists.txt: Line 106: COMMAND ${THIRDPARTY_DIR}/build-if-necessary.sh > shouldn't this only be building the uninstrumented group by default, and th You're looking at the patches out-of-order; the dependency group patch comes _after_ this one. Sorry for not making that clear. http://gerrit.cloudera.org:8080/#/c/4511/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 345: # Enable debug symbols so that stacktraces and linenumbers are available at > Are the platform standard libraries built with debug symbols? If so we may libstdc++ on my machine doesn't have debug symbols in it, no. -- To view, visit http://gerrit.cloudera.org:8080/4511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
Dan Burkert has posted comments on this change. Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/4511/2/CMakeLists.txt File CMakeLists.txt: Line 106: COMMAND ${THIRDPARTY_DIR}/build-if-necessary.sh shouldn't this only be building the uninstrumented group by default, and the tsan group if KUDU_USER_TSAN is set? http://gerrit.cloudera.org:8080/#/c/4511/2/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 82: local BUILD_TYPE=$1 (mindblown) http://gerrit.cloudera.org:8080/#/c/4511/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 345: # Enable debug symbols so that stacktraces and linenumbers are available at Are the platform standard libraries built with debug symbols? If so we may want to include them in the libc++ build. -- To view, visit http://gerrit.cloudera.org:8080/4511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: fix up libtool scripts if needed
Adar Dembo has posted comments on this change. Change subject: thirdparty: fix up libtool scripts if needed .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4512/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 383: $TP_DIR/postflight.py > This should be in the above if block, I think. It will certainly always fa Well, the goal was to be generic enough to run all the time. I think we can achieve that if the TSAN dependency check is reordered so that the check for the installed/tsan/lib precedes the check for ldd. I'll do that. -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: fix up libtool scripts if needed
Dan Burkert has posted comments on this change. Change subject: thirdparty: fix up libtool scripts if needed .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4512/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 383: $TP_DIR/postflight.py This should be in the above if block, I think. It will certainly always fail on OS X, since there is no ldd or tsan. I think it may also fail if you do a "./build-thirdparty.sh uninstrumented" on a clean checkout. -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: split into dependency groups
Adar Dembo has posted comments on this change. Change subject: thirdparty: split into dependency groups .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4513/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 141: echo TSAN does not work on OS X > This triggers with a plain "./build-thirdparty.sh" invocation Oops, will fix. Line 395: echo "Thirdparty dependencies '$*' built and installed successfully" > This isn't great with no arguments. Done -- To view, visit http://gerrit.cloudera.org:8080/4513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: split into dependency groups
Dan Burkert has posted comments on this change. Change subject: thirdparty: split into dependency groups .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4513/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 141: echo TSAN does not work on OS X This triggers with a plain "./build-thirdparty.sh" invocation Line 395: echo "Thirdparty dependencies '$*' built and installed successfully" This isn't great with no arguments. -- To view, visit http://gerrit.cloudera.org:8080/4513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Dan Burkert has posted comments on this change. Change subject: thirdparty: stifle unused argument warnings when building with clang .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Ninad Shringarpure has posted comments on this change. Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/4526/6/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: Line 219: // TODO: would be nice to include some other stuff like memory usage > Nit: please fix Tidy Bot's warning as well. You can take a look at the nex Fixed PS6, Line 238: > Nit: an extra space; clang compiler is smart enough not to take it as '>>' Fixed PS6, Line 239: > ditto Fixed Line 249: generate_table("Live Tablets",live_peers, output); > Nit: a space is missing. Fixed Line 252: *output << "Tombstoned tablets are tablets that previously " > Why not to put that after the table? If this precedes the table, then this Fixed Line 254: generate_table("Tombstoned Tablets",tombstoned_peers, output); > Nit: a space is missing. Fixed -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4526 to look at the new patch set (#7). Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section - Separated tombstoned and live tablet tables in /tablets ui page. - Added one sentence explaining tombstoned tablets as: "Tombstoned tablets are tablets that previously stored a replica on this server." - Tombstoned tablets table is displayed only when they are found. Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 --- M src/kudu/tserver/tserver-path-handlers.cc 1 file changed, 64 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/4526/7 -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] c++ client: stop requiring the old gcc ABI
Dan Burkert has posted comments on this change. Change subject: c++ client: stop requiring the old gcc ABI .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] thirdparty: split into dependency groups
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4513 to look at the new patch set (#2). Change subject: thirdparty: split into dependency groups .. thirdparty: split into dependency groups The monolithic thirdparty build is now quite a bit larger than it used to be on account of the extra LLVM build. Let's see if we can't speed it up. The idea is simple: carve it up into disjoint sections so that individual sections can be rebuilt as needed. This patch separates the various portions of the thirdparty build into "dependency groups". Conceptually, a dependency group is a set of dependencies built a certain way, but the implementation is really just a set of non-overlapping code fragments in build-thirdparty.sh. The initial set of groups are: - common: dependencies that are never instrumented. - uninstrumented: dependencies that may be instrumented, but aren't in this build. - tsan: dependencies that may be instrumented, and are indeed in this build (with -fsanitize=thread). These three generally map to the existing "common", "uninstrumented", and "tsan" thirdparty subdirectories. There's an obvious pattern here for future sanitizer builds (e.g. MSAN would provide an "msan" dependency group). The new build-if-necessary.sh can accept an argument that maps to a set of dependency groups representing a particular build. Every dependency group has its own hash/stamp file so that it is only rebuilt when needed. This also fixes a bug in the stamp file approach that prevented it from actually rebuilding anything. Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 --- M CMakeLists.txt M build-support/jenkins/build-and-test.sh M thirdparty/.gitignore M thirdparty/build-if-necessary.sh M thirdparty/build-thirdparty.sh 5 files changed, 260 insertions(+), 202 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4513/2 -- To view, visit http://gerrit.cloudera.org:8080/4513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] c++ client: stop requiring the old gcc ABI
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4515 to look at the new patch set (#2). Change subject: c++ client: stop requiring the old gcc ABI .. c++ client: stop requiring the old gcc ABI With the upgrade to clang 3.9 and the transition to libc++ for TSAN, we can now safely remove the old gcc ABI guard rail without breaking TSAN builds. The impact on backwards compatibility is immaterial. At least one Kudu vendor has shipped a client package for Ubuntu 16.04 built against the old ABI; that package will be built against the new ABI going forward. Any application built against the old ABI will be incompatible with said package once the ABI changes. But, c++ client consumers are few and far between, and the major consumer of record (Apache Impala) does not yet build on Ubuntu 16.04. Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381 --- M .ycm_extra_conf.py M CMakeLists.txt M docs/release_notes.adoc M python/setup.py M src/kudu/client/client.h M src/kudu/client/client_samples-test.sh M src/kudu/client/samples/CMakeLists.txt M thirdparty/build-thirdparty.sh 8 files changed, 9 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/4515/2 -- To view, visit http://gerrit.cloudera.org:8080/4515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4514 to look at the new patch set (#2). Change subject: thirdparty: stifle unused argument warnings when building with clang .. thirdparty: stifle unused argument warnings when building with clang Dan tested my various thirdparty patches on macOS and reported that clang emits thousands of unused argument warnings when building llvm, gflags, and gtest. The first is due to the use of EXTRA_LDFLAGS in the llvm build; the others have always been there. We can tackle this in one of two ways: 1. Add -Qunused-arguments to EXTRA_CXXFLAGS. This isn't as easy as it sounds, because we need to restrict this to clang-based builds (gcc doesn't recognize the parameter), and sometimes that happens via CC/CXX environment variables and other times implicitly. 2. Narrow our prolific additions of -L... and -Wl,-rpath,... such that they're only added when needed. I chose approach #2, which also meant remembering all of our interdependencies. As best I can tell, here is the complete list: - glog depends on gflags - glog depends on libunwind - pmemobj depends on pmem - bitshuffle depends on lz4 With such a short list, approach #2 isn't actually that bad. I tested it by building thirdparty with my system's clang. It worked once I disabled -Werror in the nvml build. I tried very hard not to regress commit 2567ed0. My system should be using gold, though I noticed that only the shared objects in installed/tsan had RUNPATH entries; the ones in installed/uninstrumented and installed/common used RPATH. Nevertheless, I ran both debug and tsan tests, which should have exercised all of them. Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779 --- M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh 2 files changed, 36 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/4514/2 -- To view, visit http://gerrit.cloudera.org:8080/4514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4511 to look at the new patch set (#2). Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds .. thirdparty: use libc++ instead libstdc++ for TSAN builds This all began because I wanted two things: 1. To use the new gcc 5 ABI on platforms that default to it (such as Ubuntu Xenial). Other applications compiled on these platforms will use the new ABI, and the fact that the Kudu client forces them to use the old ABI is quite unfriendly. 2. To have working local TSAN builds again, which broke following the gcc 5 ABI transition in Xenial. There are a number of interconnected issues at play: A. Until 3.9, LLVM did not recognize gcc's new ABI tags, which prevented Kudu's codegen module from building properly against the new ABI. B. For TSAN builds, we rebuild some thirdparty dependencies against the libstdc++ from thirdparty, but the LLVM libraries are not one of them. This may work when the system libstdc++ and the thirdparty libstdc++ are of the same version, but becomes increasingly untenable as the versions differ. Why? Because libstdc++ only guarantees forward compatibility; that is, a binary linked against libstdc++ can only be used with a libstdc++ of the same version or newer. Attempts to run with an older libstdc++ can lead to run time errors about missing GLIBCXX symbols. As a point of reference, on Xenial the two libraries are more than a major version apart. C. Continuing B, libstdc++ from gcc 5 actually breaks backward compatibility for certain C++11-only symbols by moving them to an inline namespace (e.g. std::error_category is now std::_V2::error_category). The LLVM libraries use these symbols, which means LLVM built against a gcc 5 libstdc++ cannot link against the older libstdc++ in thirdparty. D. As the libstdc++ in thirdparty is from gcc 4, it is not multilib and does not provide new ABI symbols (e.g. std::__cxx11::string). Meaning, if the rest of Kudu tried to use the new ABI, TSAN builds would fail because the libstdc++ in use lacks new ABI symbols. After upgrading LLVM, the path of least resistance was to upgrade libstdc++ in thirdparty, but what a saga that turned out to be. After much trial and error, I gave up; I could not build libstdc++ from gcc 5 with clang, and we must use clang to realize the latest -fsanitize=thread support. Are there any alternatives? Well, we can follow Chromium's lead and use libc++ for TSAN instead of libstdc++. I think this makes sense for several reasons: - The LLVM build, such as it is, is much more friendly than gcc's build. Building libstdc++ out of all of gcc was always a little hacky. - There's at least one large open-source project (Chromium) that's successfully gone down this path. That brings us to this patch, which is largely about replacing libstdc++ with libc++. Here are additional interesting details: o We now build entire set of TSAN-duplicated dependencies with -fsanitize=thread, not just protobuf. It doesn't affect correctness much either way, but it's simpler and an easier concept to extend to future sanitizers that DO care (e.g. MSAN). o We now build LLVM twice: once against the system libstdc++ for build tools and the regular LLVM libraries, and a second time against libc++ for instrumented LLVM libraries. The first build is a little hokey: it'd be more "pure" to build LLVM three times: once for build tools, once for LLVM libraries, and once for instrumented LLVM libraries. But these builds are super long so we optimize by combining the first two. The downside is that the first build now places build tools in 'installed-deps' instead of 'installed'. I played around with placing build tools in 'installed' while placing the libraries in 'installed-deps', but found that to be too hacky. o The full thirdparty build is now quite a bit longer on account of the second LLVM library build. I tried to mitigate this by reducing the number of extra cruft built each time. An upcoming patch will address this further by splitting thirdparty into separate modules. o libc++ depends on libc++abi, so we build that first. o The libc++ and libc++abi builds are done standalone rather than with the LLVM library build, because it isn't possible to do them together AND have the LLVM libraries depend on libc++. o build_python may now be invoked more than once, so I've changed it to be idempotent within the same run of build-thirdparty.sh. Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895 --- M CMakeLists.txt M build-support/dist_test.py M build-support/run-test.sh M build-support/run_dist_test.py M cmake_modules/FindPmem.cmake M src/kudu/codegen/CMakeLists.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh D th
[kudu-CR] thirdparty: fix up libtool scripts if needed
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4512 to look at the new patch set (#2). Change subject: thirdparty: fix up libtool scripts if needed .. thirdparty: fix up libtool scripts if needed Older versions of libtool (such as the version found on el6) do not pass -stdlib=libc++ found in CXXFLAGS down to the libtool link command line, and as a result, the shared object is linked against the system libstdc++. The net effect is that some thirdparty shared objects sprout @@GLIBCXX dependencies. Mysteriously, Kudu libraries and binaries themselves do not, and the @@GLIBCXX dependencies appear to be satisfied via libc++ somehow. Nevertheless, this is confusing and could prove problematic down the road, so here's a quick hack to "fix up" libtool scripts after they are generated via configure. Change-Id: Id51c10d38984e892496621135634d21f3e2386ce --- M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh A thirdparty/postflight.py 3 files changed, 131 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4512/2 -- To view, visit http://gerrit.cloudera.org:8080/4512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Ninad Shringarpure has posted comments on this change. Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/4526/6/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: PS6, Line 191: this > Why is 'this' capture necessary for this functor? This lambda function is accessing ConsensusStatePBToHtml method. Not the right way to do it? -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: reorganize tree
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4550 to look at the new patch set (#2). Change subject: thirdparty: reorganize tree .. thirdparty: reorganize tree This patch changes the organization of the thirdparty tree. The new layout looks like this: - installed: _all_ installed dependencies, with 'common', 'uninstrumented', and 'tsan' subdirectories. - build: build directories for all dependencies. - src: source directories for all dependencies. Additionally, the patch changes the build logic for each dependency so that its build output is fully isolated from its source directory, and from other build output (if the dependency is built multiple times). Why do this? 1. Build isolation simplifies building dependencies multiple times (i.e. for different sanitizers) and makes it much safer. 2. It also means cleaning up build output doesn't mean redownloading all of the sources (i.e. no need to 'git clean -xdf thirdparty'). 3. The grouping of all installed locations under the shared 'installed' subdirectory makes it a tad easier to blow it all away. 4. It also eases the transition for the remaining LLVM and thirdparty patches, as the conflicting build output will be copied to a different set of directories. 5. It slims down thirdparty/.gitignore; adding a new dependency no longer requires updating .gitignore. Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383 --- M .ycm_extra_conf.py M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh M build-support/lint.sh M build-support/run_dist_test.py M docs/installation.adoc M docs/support/scripts/make_site.sh M java/kudu-client/pom.xml M src/kudu/client/client_samples-test.sh M src/kudu/scripts/benchmarks.sh M src/kudu/scripts/tpch.sh M thirdparty/.gitignore M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 334 insertions(+), 217 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/4550/2 -- To view, visit http://gerrit.cloudera.org:8080/4550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: reorganize tree
Adar Dembo has posted comments on this change. Change subject: thirdparty: reorganize tree .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/4550/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 1: #!/bin/bash > Is this necessary? Every other shell script in the Kudu repo uses it, so I figure it's best to be consistent. Plus, I might be introducing bash-isms with these changes, so better safe than sorry. Line 43: restore_env() { > Should this now include MODE_SUFFIX? I think that might be good so that we Ahh, didn't realize PREFIX was also saved/restored. Yeah, I'll add MODE_SUFFIX too. Line 439: rsync -a $BOOST_SOURCE/boost $PREFIX/include > Not your change, but this rsync incantation is using different flags than t You referring to the lack of -v, or --delete? The former is because boost tree is stupendously huge, so logging every file (-v) is kind of a drag. But --delete should probably go in there, otherwise we may not notice header removals during upgrades. I'll add it. -- To view, visit http://gerrit.cloudera.org:8080/4550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] thirdparty: clean up llvm cmake files too
Adar Dembo has submitted this change and it was merged. Change subject: thirdparty: clean up llvm cmake files too .. thirdparty: clean up llvm cmake files too The upgrade to LLVM 3.9 changed the list of targets provided in LLVM's cmake files. If the cmake files are left behind and make it into an LLVM 3.8-based build, the subsequent Kudu build will fail because it can't find all of the targets listed. It's too late to help with the LLVM 3.9 upgrade, but hopefully it'll ease future upgrades. Change-Id: I98393cf1f8afc2ced78245cad5e2e24ee9410214 Reviewed-on: http://gerrit.cloudera.org:8080/4549 Reviewed-by: Dan Burkert Tested-by: Kudu Jenkins --- M thirdparty/build-definitions.sh 1 file changed, 2 insertions(+), 1 deletion(-) Approvals: Dan Burkert: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I98393cf1f8afc2ced78245cad5e2e24ee9410214 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Adar Dembo has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/kudu-test.cc File src/kudu/tools/kudu-test.cc: Line 37: > The exact row count is verified by the utility itself if adding '-run_scan' Ah, forgot about -run_scan. No, then this is fine. -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Alexey Serbin has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 15: (5 comments) http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: PS15, Line 108: ADD_KUDU_TEST(kudu-test) : ADD_KUDU_TEST_DEPENDENCIES(kudu-test : kudu) > Why not reuse kudu-tool-test, which is where all of the new CLI tests (apar Good idea, will do. I was not sure where to put it; probably I should have asked for an advice. http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/kudu-test.cc File src/kudu/tools/kudu-test.cc: Line 37: > These are nice, but can we at least verify that something was written with The exact row count is verified by the utility itself if adding '-run_scan' flag (this is so for all tests besides the very first one which runs with default parameters). Do you mean we want to verify the exact data which has been written? I.e., read it back and compare with the data which was inserted into the table? http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: Line 110: #include "kudu/util/random.h" > Nit: should precee stopwatch (surprised Tidy Bot didn't mention it?) Good catch, will update. So, it seems there is a hope -- not all our jobs are going to disappear due to automation and AI :) Probably, there is a bug in Tidy Bot. I'm just speculating here, but it might be related to the fact that this header comes last in the list (it might be off-by-one mistake or alike). PS15, Line 152: options allows " : "to keep > Nit: option retains Done PS15, Line 156: is not in effect > Nit: has no effect Done -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Adar Dembo has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: > that being said, maybe you could just insert the block that causes > the test somewhere else. How about FullStackInsertScanTest, would > it be easier to cause it there? I too have a (weak) opinion that the test should be merged but I'll defer to Dan. That said, we use FullStackInsertScanTest in performance dashboards, so I don't think we should add a FsManager.Open() looping thread to it. -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: (1 comment) that test is going to bit rot super quickly and this seems like a thorny enough problem that we'd want to run it frequently in case we break it somewhere else. 60 seconds isn't much when running tests in parallel. that being said, maybe you could just insert the block that causes the test somewhere else. How about FullStackInsertScanTest, would it be easier to cause it there? http://gerrit.cloudera.org:8080/#/c/4551/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 496: if (data_file_size < record.offset() + record.length()) { PREDICT_FALSE to annotate that this is a rare race -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Adar Dembo has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 15: (5 comments) http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: PS15, Line 108: ADD_KUDU_TEST(kudu-test) : ADD_KUDU_TEST_DEPENDENCIES(kudu-test : kudu) Why not reuse kudu-tool-test, which is where all of the new CLI tests (apart from the ported ones in kudu-ts-cli-test and kudu-admin-test) live? http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/kudu-test.cc File src/kudu/tools/kudu-test.cc: Line 37: These are nice, but can we at least verify that something was written with a scan? Can we verify an exact row count too? http://gerrit.cloudera.org:8080/#/c/4412/15/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: Line 110: #include "kudu/util/random.h" Nit: should precee stopwatch (surprised Tidy Bot didn't mention it?) PS15, Line 152: options allows " : "to keep Nit: option retains PS15, Line 156: is not in effect Nit: has no effect -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Dinesh Bhat has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4551/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS2, Line 499: in order to account for the latest appends Good find Dan, this is not a review but a curiosity Qn: Looking at the description/comments it seems like a racy situation between reader thread and a writer thread(I may have understood this wrong). Is there a guarantee here that the data_file_->Size is not changed after we grabbed the local_record.back() to inspect with CheckBlockRecord? PS2, Line 504: local_records.back() Nit: replace with record ? -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section
Alexey Serbin has posted comments on this change. Change subject: [web-ui] KUDU-1588 /tablets page should separate out tombstoned tablets to a separate section .. Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/4526/6/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: PS6, Line 191: this Why is 'this' capture necessary for this functor? Line 219: // TODO: would be nice to include some other stuff like memory usage > warning: missing username/bug in TODO [google-readability-todo] Nit: please fix Tidy Bot's warning as well. You can take a look at the next line for the hint if you don't know who added this TODO. PS6, Line 238: Nit: an extra space; clang compiler is smart enough not to take it as '>>' operator. PS6, Line 239: ditto Line 249: generate_table("Live Tablets",live_peers, output); Nit: a space is missing. Line 252: *output << "Tombstoned tablets are tablets that previously " Why not to put that after the table? If this precedes the table, then this paragraph is attributed to the 'Live Tablets': Live Tablets ... ... Tombstoned Tablets ... Line 254: generate_table("Tombstoned Tablets",tombstoned_peers, output); Nit: a space is missing. -- To view, visit http://gerrit.cloudera.org:8080/4526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0aa9b67749234c5d68899a82d6b1493633bb78c5 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ninad Shringarpure Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Ninad Shringarpure Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] Implement a manual leader step down for a tablet
Adar Dembo has posted comments on this change. Change subject: [tools] Implement a manual leader_step_down for a tablet .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: PS1, Line 204: } > Sure, could you tell me what's the relationship between this being a main t The CHECK_* macros come from glog, and they all do the same thing regardless of the context in which they're called: 1. Evaluate an expression. 2. If evaluation fails, log something. 3. Dump a stack trace. 4. Call abort() to exit the process. The DCHECK_* macros (also from glog) are debug-only variants that are otherwise equivalent, but compiled out in non-debug builds. The ASSERT_* macros come from gtest, so they're specific to how gtest works. They too evaluate an expression, but on failure, they do some gtest stuff to capture the failure, end the run of the current test, and move to the next test. They have some foibles, and one of them is that ASSERT failures in the thread that isn't the main process thread don't seem to get noticed by gtest. Maybe there's something we need to do for that to happen; if there is, I don't know what it is. Oh, and EXPECT_* is like ASSERT_* except that the test keeps running. So ASSERT_* will only ever reveal the "first" failure, while EXPECT_* will reveal all of them. So to summarize: - Use CHECK_*/DCHECK_* macros in production code. Use DCHECK_* if you expect to get sufficient test coverage of the callsite, CHECK_* otherwise. - Use ASSERT_*/EXPECT_* macros in test-only code, unless that code is expected to run on a separate test thread, in which case use CHECK_* too. Default to EXPECT_*, unless the operation in question must succeed for the rest of the test to function properly, in which case use ASSERT_*. Hope this helps. Line 218: ASSERT_OK(Subprocess::Call({ > In reality yes, but given that we are operating in a deterministic test env Unfortunately a test environment with leader failure detection isn't deterministic. The test can run on a machine with arbitrary load (perhaps from other tests), in which case one of the tserver processes can pause for arbitrary periods of time, causing a new leader to be elected. This is one of the leading causes of (low-grade) test flakiness for us. PS1, Line 230: CHECK_OK(GetInfoFromConsensus(tservers, tablet_id_, : &new_leader, &new_term)); > I tried this with an unsuccessful result, later realized AssertEventually() Couple things: 1. I don't understand why AssertEventually() doesn't work here. AFAICT the goal of this snippet is "tell the leader to step down, wait for an election to happen, then assert that the new term is higher than the one before". If you wrap L229-L233 in AssertEventually(), doesn't that mean you don't need the 3 second sleep (which could be flaky to begin with, since a cluster running with ASAN/TSAN could take a lot longer to run an election)? I don't see what the issue is; can you help me understand? Look at some of the existing usages of AssertEventually() if it's not clear. 2. I'm not seeing the bug in the code you pasted. What is it? Line 264: s = GetInfoFromConsensus(tservers, tablet_id_, > Done The actual check (not in the CHECK sense of the word, but the comparison and assertion) is still here, though. http://gerrit.cloudera.org:8080/#/c/4533/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: PS2, Line 251: CHECK_OK Still got some leftover CHECK_* statements that should be converted to their ASSERT_* equivalents. http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: PS1, Line 194: > Done, although not sure if we need a toggle given that the routine anyways On second thought, the ResolveAddresses output isn't even used, is it? So let's drop it altogether. Looks like address resolution happens within BuildProxy() so that extra call was redundant from day one (in ConfigChange() as well). PS1, Line 274: : > Nit: reformat as "Force the tablet's leader replica (if present) to step do You missed these comments down here. -- To view, visit http://gerrit.cloudera.org:8080/4533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1363: Add IN-list predicate type
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/2986 to look at the new patch set (#19). Change subject: KUDU-1363: Add IN-list predicate type .. KUDU-1363: Add IN-list predicate type Adds support in the C++ client for providing a set of equalities for a given column. Support for using IN list predicates from the Java client will be in a follow-up commit. Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/predicate-test.cc M src/kudu/client/scan_predicate-internal.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/value.h M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/column_predicate.h M src/kudu/common/common.proto M src/kudu/common/key_util.cc M src/kudu/common/scan_spec-test.cc M src/kudu/common/scan_spec.cc M src/kudu/common/wire_protocol-test.cc M src/kudu/common/wire_protocol.cc 16 files changed, 1,000 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/19 -- To view, visit http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1363: Add IN-list predicate type
Dan Burkert has posted comments on this change. Change subject: KUDU-1363: Add IN-list predicate type .. Patch Set 18: (3 comments) http://gerrit.cloudera.org:8080/#/c/2986/18/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 464: // = | | > nit: is this supposed to be Done Line 472: // <) AND > Just in case, consider adding tests for boundary conditions like The equality ones are already tested in the equality + range section. I've added some additional tests to make sure single element lists get simplified to an equality. Added the rest. http://gerrit.cloudera.org:8080/#/c/2986/18/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: PS18, Line 357: ASSERT_EQ > Here and below for the ASSERT_EQ() macro: Done -- To view, visit http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4412 to look at the new patch set (#15). Change subject: [tools] added insert-generated-rows into kudu tools .. [tools] added insert-generated-rows into kudu tools The insert-generated-rows tool has been merged into the 'kudu' umbrella toolset. This addresses KUDU-1628. Besides, added ability to run multiple inserter threads and specify additional parameters on batching behavior of the generated write operations. It's possible to run data generating sessions both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes. Introduced sequential and random modes for the data generator. Overall, these changes allow to use the tool to measure performance of the Kudu C++ client library in simplistic 'push-as-much-as-you-can' scenario: the client generates and sends data as fast as it can. Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad --- M src/kudu/tools/CMakeLists.txt D src/kudu/tools/insert-generated-rows.cc A src/kudu/tools/kudu-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/tools/tool_action_remote_replica.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc A src/kudu/tools/tool_action_test.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_action_wal.cc M src/kudu/tools/tool_main.cc 17 files changed, 758 insertions(+), 179 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/15 -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Adar Dembo has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] docs: add master permanent failure recovery workflow
Adar Dembo has submitted this change and it was merged. Change subject: docs: add master permanent failure recovery workflow .. docs: add master permanent failure recovery workflow While testing this I filed KUDU-1620; this wasn't an issue in master_failover-itest because it (obviously) can't do any DNS aliasing. Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95 Reviewed-on: http://gerrit.cloudera.org:8080/4436 Reviewed-by: David Ribeiro Alves Tested-by: Adar Dembo --- M docs/administration.adoc 1 file changed, 165 insertions(+), 14 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/4436 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves
[kudu-CR] docs: add master permanent failure recovery workflow
Adar Dembo has posted comments on this change. Change subject: docs: add master permanent failure recovery workflow .. Patch Set 2: Verified+1 Overriding Jenkins, the build failures were due to other thirdparty patches in flight and not to do with this doc change. -- To view, visit http://gerrit.cloudera.org:8080/4436 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4412 to look at the new patch set (#14). Change subject: [tools] added insert-generated-rows into kudu tools .. [tools] added insert-generated-rows into kudu tools The insert-generated-rows tool has been merged into the 'kudu' umbrella toolset. This addresses KUDU-1628. Besides, added ability to run multiple inserter threads and specify additional parameters on batching behavior of the generated write operations. It's possible to run data generating sessions both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes. Introduced sequential and random modes for the data generator. Overall, these changes allow to use the tool to measure performance of the Kudu C++ client library in simplistic 'push-as-much-as-you-can' scenario: the client generates and sends data as fast as it can. Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad --- M src/kudu/tools/CMakeLists.txt D src/kudu/tools/insert-generated-rows.cc A src/kudu/tools/kudu-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/tools/tool_action_remote_replica.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc A src/kudu/tools/tool_action_test.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_action_wal.cc M src/kudu/tools/tool_main.cc 17 files changed, 769 insertions(+), 179 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/14 -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Dan Burkert has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: Two reasons: It's typically about 35 seconds to repro on ve0158; when we fix it we'll have to run the test for perhaps 60 seconds to really make sure it doesn't happen. The issue already has pretty good coverage through alter_table-randomized-itest. -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Dinesh Bhat has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/4412/12/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: PS12, Line 452: t of master a > The original approach was to be able to use any table structure, not only s Yeah, my comment at L457 was kinda related to this. i.e, I am not sure if we need to be flexible wrt schemas - we can have one static schema table and load generate and delete the table at the end of the test with an option to keep it if we want to append rows later. Basically when it comes to tests, one default approach could be 'leave_no_trace' philosophy. Imagine if I run this tool continuously for 100 times without exposing a button to delete the data generated from these tests, which means we are growing the data in unbounded fashion. PS12, Line 455: et > Yes, here and elsewhere in the description for parameters: this is intentio Other tools seem to have followed just one space, so was trying to be consistent with them. -- To view, visit http://gerrit.cloudera.org:8080/4412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] docs: add master permanent failure recovery workflow
David Ribeiro Alves has posted comments on this change. Change subject: docs: add master permanent failure recovery workflow .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4436 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] WIP: consensus: refactor tracking of received OpIds out of ReplicaState
David Ribeiro Alves has posted comments on this change. Change subject: WIP: consensus: refactor tracking of received OpIds out of ReplicaState .. Patch Set 1: I like the consolidation and, as I had said on the mailing list, I'm +1 on the removal of the test, so, in general, I think this patch is a good idea. -- To view, visit http://gerrit.cloudera.org:8080/4476 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I81614d26328b0fbba37bf279f59717e05a07b816 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1612 - [python] Enable setting of read mode for scanning
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1612 - [python] Enable setting of read mode for scanning .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4520/2/python/kudu/tests/util.py File python/kudu/tests/util.py: Line 122: self.snapshot_timestamp = datetime.datetime.utcnow() > Since this op is being flushed why would it not be guaranteed? Since its bl because the server's clock might not have the same time as the system's clock (it's not a pure physical clock). i.e. the time returned by the system clock might behind or ahead of the time the server assigned to the write. it's unlikely but it can happen and thus it would make this test flaky. -- To view, visit http://gerrit.cloudera.org:8080/4520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c61ef09f6e15bad2c44d9caf85b2cc2582b8a49 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1363: Add IN-list predicate type
Alexey Serbin has posted comments on this change. Change subject: KUDU-1363: Add IN-list predicate type .. Patch Set 18: (3 comments) http://gerrit.cloudera.org:8080/#/c/2986/18/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 464: // = | | nit: is this supposed to be [---> | | | = | | ? I hope you are reading this comment in a monospace font :) Line 472: // <) AND Just in case, consider adding tests for boundary conditions like [--) AND | [--) AND || [--) AND | | Also, I didn't notice that the following cases are covered: [) AND | [) AND | | <-) AND | <-> AND | MAX_INT <-) AND | Consider adding those as well. Besides, a tiny nit: consider re-grouping the test cases so the range-related ones come in one block (if, of course, there isn't any other grouping criterion in effect here which I might be missing). http://gerrit.cloudera.org:8080/#/c/2986/18/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: PS18, Line 357: ASSERT_EQ Here and below for the ASSERT_EQ() macro: if I'm not mistaken, the expected value is the first parameter, the actual is the second. -- To view, visit http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 2: why not include the test and only run it in slow mode? jenkins runs tests with dist-test so I don't think that 30 secs is going to be a problem there. -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-861 Support changing default, storage attributes
Dan Burkert has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/4310/6/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: Line 147: ByteString defaultByteString = ProtobufHelper.objectToByteStringNoType(name, newDefault); > Yeah, but that's not a good excuse for me not to at least try and improve i After thinking about this a bit more (and hitting a similar issue with the IN list stuff), I'm not sure how it could be realistically improved. Adding a KuduValue type interface doesn't really make the situation any better; there is still the opportunity for a type mismatch with the column. Probably best to leave as-is for now. http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: Line 106: RETURN_NOT_OK(s.spec->ToColumnSchemaDelta(&col_delta)); This does some of the checks above, right? Maybe it would be best to consolidate all of them in ToColumnSchemaDelta. http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: PS6, Line 353: gscoped_ptr > Done. Is there a goal to remove gscoped_ptr in favor of unique_ptr througho Yes, I think long term. It's hairy to update that much code at once, though. For now we are just trying to not introduce more usage of gscoped_ptr. -- To view, visit http://gerrit.cloudera.org:8080/4310 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1563. Add support for INSERT IGNORE
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1563. Add support for INSERT IGNORE .. Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: Line 80: using client::KuduColumnSchema; > warning: using decl 'KuduColumnSchema' is unused [misc-unused-using-decls] not your fault, but could you address these warnings? hard to keep track of the new ones versus the old ones. you can leave out the TODO(username) ones that are not introduced by this patch. Line 411: LOG(FATAL) << "r = " << r; can you add more info to the output? i.e. what is "r". the row key might also be helpful http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet-test-base.h File src/kudu/tablet/tablet-test-base.h: Line 326: // Inserts "count" rows. add: "... , ignoring errors in the case of duplicate keys." or something like that. PS7, Line 354: if (i % 2 == 0) { : CHECK_OK(writer.Insert(row)); : } else { : CHECK_OK(writer.InsertIgnore(row)); : } : } not sure about this block. What are you trying to do here. You've clearly already added a new op type. Are you trying to make it so that most tests, which use regular inserts, also test the insert ignore code path? I like the idea, but not sure this is the best way to go. doesn't this make some tests fail (and if not shouldn't it?). http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 401: if (present) { would still rather have a switch here (this was one of the intentions of the original comment, though I now notice I didn't mention it at all, guess you didn't read my mind :) ). Line 407: op->SetInsertIgnoreSucceeded(); this method name here reads even weirder. How about: op->SetInsertErrorIgnored() or something http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 365: // refer to INSERT IGNORE. It refers to inserts ignored during log replay add: "... purposefully ignored due to duplicate key errors during log replay" or something like that. http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: PS2, Line 50: scoped_refptr rows_insert_ignored; > Yeah I can see folding this into rows_Inserted. At the same time, I like mo how about renaming the var to something like: insert_errors_ignored. or something of the kind -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Brock Noland Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Dan Burkert has posted comments on this change. Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4551/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS1, Line 498: // This codepath can be used in read-only mode as part of a tool running : // against a live tablet server. In this case, the tablet server may be : // actively appending entries to the container, so the data_file_size may : // need to be occasionally updated to account for new data entries. : // CheckBlockRecord will crash the process when data_file_size is less : // than what the metadata record indicates. > Nit: can you reword this comment to reduce its scope? We're deep inside a b Done Line 504: > Nit: remove this empty line? Done -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4551 to look at the new patch set (#2). Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash .. KUDU-1657: read-only FsManager::Open on active tablet can crash See the comment and JIRA for explanation. No test is included, since the only reproducible case takes approximately 30 seconds to run. alter_table-randomized-test is made significantly flaky by this issue, so it has some amount of coverage already. The repro creates a single tablet with many columns and no replication, and writes data to it as fast as possible. Another thread creates a file system manager in a loop, which opens a log block manager on the local tablet. The repro can be found at: https://github.com/danburkert/kudu/commit/e919ca410941b268993824da7668ee69c5a7b31c Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 --- M src/kudu/fs/log_block_manager.cc 1 file changed, 11 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/4551/2 -- To view, visit http://gerrit.cloudera.org:8080/4551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: reorganize tree
Dan Burkert has posted comments on this change. Change subject: thirdparty: reorganize tree .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/4550/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 1: #!/bin/bash Is this necessary? Line 43: restore_env() { Should this now include MODE_SUFFIX? I think that might be good so that we will fail fast if we try to use it without setting it. Although now that I'm looking, I don't think we set -u, so it may not make a difference. Line 439: rsync -a $BOOST_SOURCE/boost $PREFIX/include Not your change, but this rsync incantation is using different flags than the others, is that intentional? -- To view, visit http://gerrit.cloudera.org:8080/4550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [tools] added insert-generated-rows into kudu tools
Alexey Serbin has posted comments on this change. Change subject: [tools] added insert-generated-rows into kudu tools .. Patch Set 12: (20 comments) http://gerrit.cloudera.org:8080/#/c/4412/11/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: PS11, Line 358: } : if (total_row_count != nullptr) { : *total_row_count = accumulate(row_count.begin(), row_count.end(), 0UL); : } > Looks like it's still here? Oops, my bad. I'm surprised myself -- probably, I got distracted. Let me remove it, though. http://gerrit.cloudera.org:8080/#/c/4412/12/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: PS12, Line 21: options > s/options/option ? Done Line 33: // --num_threads=1 \ > don't you need master_addresses here as required params ? also applicable i Good catch -- these examples became stale once I switched to the mandatory master(s) location. Will update PS12, Line 75: master server(s) location > as per the code below there is no default_params for master server location Yes, correct -- since changing to use master addresses as mandatory parameter, this no longer has default value. Will update. Line 80: // bench_c_00_03 > Same as above. Also here and above, we could keep this name intuitive to re Done Line 119: using kudu::Schema; > I think Schema is not being used anywhere. Done Line 131: using kudu::client::sp::shared_ptr; > Actually a basic Qn here: why do we use this special shared_ptr as opposed It's just a typedef: it might be either std::shared_ptr or std::tr1::shared_ptr, check $REPO_ROOT/src/kudu/client/shared_ptr.h That's because Kudu C++ client API uses this notation to be compatible with C++03. PS12, Line 142: 100 > Nit: Not sure if you want to insert 1 million by default, if I am a curious 1M rows are inserted really fast: at my laptop it's just 5 seconds. Also, the idea was to create some _load_ :) With 1K rows there is no load at all. Besides, take into the consideration that the original insert-generated-rows tools was running with no limit at generated rows at all. So, what do you think would be the right number here? Line 194: int64_t NextImpl() { > Do we want to use uint64_t here ? Although Random.Next64() seems to be fill OK, that makes sense. Will update to uint64_t. PS12, Line 215: != 0 > this check is prolly redundant ? As I remember, Tidy Bot thinks it's better for readability: that's the reason I left it here. I'll try to get rid of it -- let's see what it will say. PS12, Line 285: case UNKNOWN_DATA: // fall-through > redundant ? Yes, it seems so. I copied the switch enumerators from some place and left this UNKNOWN_DATA as is. Will remove. Line 302: FLAGS_buffer_flush_watermark_pct)); > I vaguely recall cpp guideline saying next spill being after 4 spaces I gue QtCreator's editor does this sometimes. Will fix. PS12, Line 314: FLAGS_num_rows_per_thread > consider using variable FLAGS_ directly to be consistent with other places. I wanted to have a local variable since I access it in multiple places. PS12, Line 315: num_rows_per_thread == 0 > given it's default value being 1M, what's this check for ? Take a look at the comment for the '--num_rows_per_thread': 0 means no limit. Line 324: if (row_count != nullptr) { > DCHECK or CHECK for this, unless we have a use case where it could actually I don't see why DCHECK or CHECK is needed here. One can pass nullptr as rowcount: that means there isn't any interest in getting the count. It's a ubiquitous notation for pointer out parameters, which I want to have here, even if nobody uses it right now. Line 403: Stopwatch sw(Stopwatch::ALL_THREADS); > Stupid Qn: Does ALL_THREADS here mean all threads on CPU or all threads gen Threads in this process, so the monitoring/main thread will also be counted in. I don't want LOG_TIMING, since I want to output that information in my own format, not what LOG_TIMING provides. PS12, Line 440: optional scan afterwards > I think this need not be made explicit since this is listed in optional par I want this to be present to emphasize that there are two parts: the write and the read one, even if the latter is optional. That's an essence of this test in a terse phrasing, otherwise it would be necessary to read through all the list of options to understand about the read part. PS12, Line 452: kTableNameArg > Can we not make this table_name optional and create one if the user doesn't The original approach was to be able to use any table structure, not only some fixed pre-generated one which the test would create. So, this test retrieves the table schema out of existing table it's pointed at. That's because specifying custom table structure via parameters of this test would be cumbersome. We can add this an option, with pairing auto-delet
[kudu-CR] [tools] Implement a manual leader step down for a tablet
Dinesh Bhat has posted comments on this change. Change subject: [tools] Implement a manual leader_step_down for a tablet .. Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/4533/1//COMMIT_MSG Commit Message: PS1, Line 12: > Nit: got an extra space here. Done PS1, Line 18: tablet > You mean 'table' here, right? yeah, thanks for catching. http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: PS1, Line 178: Status s = itest::GetConsensusState(ts, : tablet_id, : consensus::CONSENSUS_CONFIG_COMMITTED, : MonoDelta::FromSeconds(10), &cstate); : if (!s.ok()) { : return s; : } > Just use RETURN_NOT_OK(...). Done Line 198: int num_retries = 0; > Why is this defined way up here instead of closer to where it's used? Done, was thinking of keeping the *retries* variables together initially, but I agree about moving it down just before the use. Also just made it one variable now. PS1, Line 204: CHECK_EQ > Use ASSERT_* instead of CHECK_* since all of these are on the main test thr Sure, could you tell me what's the relationship between this being a main thread and these macros here ? I remember asking this to you earlier in one of earlier rev comments but forgot to follow up in-person. I am just curious if there are debug-only versions of these macros ? Line 218: CHECK_EQ(master_reported_leader, current_leader); > The leader can change between the GetInfoFromConsensus calls and L213, righ In reality yes, but given that we are operating in a deterministic test environment where the server failures are manually triggered by code, I thought this check is safe to have. However, I am not entirely sure what this check is buying us, I just happen to notice that there are more than one way we could find the leader replica, and I thought it would be good to compare them when we know that they don't change on the fly during the test. I can remove this if you think this might be a room for any flakiness. PS1, Line 230: // Wait for re-election to happen again. : // 3 seconds picked to accommodate consensus heartbeat timeout(1.5s) > Use AssertEventually() to make this more robust, and to minimize the amount I tried this with an unsuccessful result, later realized AssertEventually() kinda doesn't seem to fit this scenario here; Reason being: a) We really want to check the consensus state after the heartbeat timeout as opposed to executing f() and then sleeping; AssertEventually() aggressively(every ms) keeps executing the f() until the timeout hits. b) I won't be able to use assert from inside the lambda argument when we know that Consensus state is still under flux. Side note: I found one minor bug in AssertEventually() here which I can fix in another patch: int attempts = 0; int sleep_ms = (attempts < 10) ? (1 << attempts) : 1000; SleepFor(sleep_ms); Perhaps we can devise another routine which sleeps for N secs and then executes the function and returns the status ? Line 264: CHECK_EQ(master_reported_leader, ""); > Let's not check this. It's a common calling convention that if a function f Done PS1, Line 271: CHECK_EQ(current_leader, ""); : CHECK_EQ(current_term, 0); > Likewise here, which means you needn't initialize current_term. Done http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: PS1, Line 194: Status s = GetTabletLeader(client, tablet_id, &leader_uuid, &leader_hp); > Consider consolidating from here to ResolveAddresses in a helper function, Done, although not sure if we need a toggle given that the routine anyways returns the Status and caller can decide if NotFound is FATAL or not ? PS1, Line 198: else if (!s.ok()) { : return s; : } > To pile on with Tidy Bot, just do RETURN_NOT_OK(s) after the not found chec Done PS1, Line 209: ServerStatusPB status; : RETURN_NOT_OK(GetServerStatus(leader_hp.host(), leader_hp.port(), &status)); > What's this for? I don't see status used anywhere. I kinda misunderstood the combination of RETURN_NOT_OK(GetServerStatus... thinking that this will return failure if leader is not up; My goal here was to check if leader is up & running so that we need not send the RPC if the server is already down. It looks like this doesn't buy us anything since RPC will fail anyways if the leader is down. Hence removed. PS1, Line 224: return Status::RemoteError(resp.ShortDebugString()); > Why not return StatusFromPB(resp.error().status()) like ConfigChange? Sounds good, just didn't know about this routine, thanks.
[kudu-CR] [tools] Implement a manual leader step down for a tablet
Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4533 to look at the new patch set (#2). Change subject: [tools] Implement a manual leader_step_down for a tablet .. [tools] Implement a manual leader_step_down for a tablet This change introduces a leader_step_down functionality under 'kudu tablet'. This tool may be handy to recover from situations when a single tablet server is overloaded and we want to kick off a new election to balance the load across the clusters. Although it is not guaranteed that a different replica will be elected as the leader, this is an optimistic effort to elect a new tablet server as the leader for the given tablet in the cluster. Also snuck in a small change to display host:port details with 'kudu table list --list_tablets' command. Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8 --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc M src/kudu/tools/tool_action_tablet.cc 3 files changed, 180 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/4533/2 -- To view, visit http://gerrit.cloudera.org:8080/4533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon