[kudu-CR] KUDU-1884: Set sasl protocol name for the TxnSystemClient

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17480 )

Change subject: KUDU-1884: Set sasl_protocol_name for the TxnSystemClient
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/17480/2//COMMIT_MSG@13
PS2, Line 13: This logic matches that in server_base.cc for server
: messenger configuration.
> Does it make sense to add a test, at least catch regressions once the overl
Probably, security-itest.cc might be even a better place for a new test 
scenario.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
Gerrit-Change-Number: 17480
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 04:43:32 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-1884: Set sasl protocol name for the TxnSystemClient

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17480 )

Change subject: KUDU-1884: Set sasl_protocol_name for the TxnSystemClient
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/17480/2//COMMIT_MSG@9
PS2, Line 9: witha
with a


http://gerrit.cloudera.org:8080/#/c/17480/2//COMMIT_MSG@10
PS2, Line 10: embeded
embedded


http://gerrit.cloudera.org:8080/#/c/17480/2//COMMIT_MSG@13
PS2, Line 13: This logic matches that in server_base.cc for server
: messenger configuration.
Does it make sense to add a test, at least catch regressions once the 
overlooked place for the kerberos principal customization is found?  It seems a 
small end-to-end scenario might be added into client-test.cc or 
txn_write_ops-itest.cc



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
Gerrit-Change-Number: 17480
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 04:42:00 +
Gerrit-HasComments: Yes


[kudu-CR](branch-1.15.x) KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/17481


Change subject: KUDU-3223: management of per-table limit by kudu CLI
..

KUDU-3223: management of per-table limit by kudu CLI

Use kudu CLI to set disk_size_limit and row_count_limit on table level.

If '--enable_table_write_limit' is not set 'true' for kudu-master, kudu
CLI fails to set the limit. The table's disk_size_limit and
row_count_limit are 'N/A' when calling 'kudu table statistics'.

Only when '--enable_table_write_limit' is set 'true', can the kudu CLI
change the table limit. Use 'kudu table statistics' to check the
updated result.

Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Reviewed-on: http://gerrit.cloudera.org:8080/17444
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
(cherry picked from commit 323d8ea7312678b7a910e55ed4f74acaf3813ffc)
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 240 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/17481/1
--
To view, visit http://gerrit.cloudera.org:8080/17481
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: branch-1.15.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17481
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..

KUDU-3223: management of per-table limit by kudu CLI

Use kudu CLI to set disk_size_limit and row_count_limit on table level.

If '--enable_table_write_limit' is not set 'true' for kudu-master, kudu
CLI fails to set the limit. The table's disk_size_limit and
row_count_limit are 'N/A' when calling 'kudu table statistics'.

Only when '--enable_table_write_limit' is set 'true', can the kudu CLI
change the table limit. Use 'kudu table statistics' to check the
updated result.

Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Reviewed-on: http://gerrit.cloudera.org:8080/17444
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 240 insertions(+), 4 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 18
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 17: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 17
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 04:33:56 +
Gerrit-HasComments: No


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 17: Code-Review+2

LGTM!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 17
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 04:12:32 +
Gerrit-HasComments: No


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Hongjiang Zhang (Code Review)
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..

KUDU-3223: management of per-table limit by kudu CLI

Use kudu CLI to set disk_size_limit and row_count_limit on table level.

If '--enable_table_write_limit' is not set 'true' for kudu-master, kudu
CLI fails to set the limit. The table's disk_size_limit and
row_count_limit are 'N/A' when calling 'kudu table statistics'.

Only when '--enable_table_write_limit' is set 'true', can the kudu CLI
change the table limit. Use 'kudu table statistics' to check the
updated result.

Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 240 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/17444/17
--
To view, visit http://gerrit.cloudera.org:8080/17444
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 17
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Hongjiang Zhang (Code Review)
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..

KUDU-3223: management of per-table limit by kudu CLI

Use kudu CLI to set disk_size_limit and row_count_limit on table level.

If '--enable_table_write_limit' is not set 'true' for kudu-master, kudu
CLI fails to set the limit. The table's disk_size_limit and
row_count_limit are 'N/A' when calling 'kudu table statistics'.

Only when '--enable_table_write_limit' is set 'true', can the kudu CLI
change the table limit. Use 'kudu table statistics' to check the
updated result.

Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 240 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/17444/16
--
To view, visit http://gerrit.cloudera.org:8080/17444
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 16
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] KUDU-1884: Set sasl protocol name for the TxnSystemClient

2021-05-20 Thread Grant Henke (Code Review)
Hello Attila Bukor, Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: KUDU-1884: Set sasl_protocol_name for the TxnSystemClient
..

KUDU-1884: Set sasl_protocol_name for the TxnSystemClient

In clusters witha custom principal we need to set the
sasl_protocol_name on the embeded client within the
TxnSystemClient.

This logic matches that in server_base.cc for server
messenger configuration.

Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
---
M src/kudu/transactions/txn_system_client.cc
1 file changed, 8 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/17480/2
--
To view, visit http://gerrit.cloudera.org:8080/17480
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
Gerrit-Change-Number: 17480
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] KUDU-1884: Set sasl protocol name for the TxnSystemClient

2021-05-20 Thread Grant Henke (Code Review)
Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/17480


Change subject: KUDU-1884: Set sasl_protocol_name for the TxnSystemClient
..

KUDU-1884: Set sasl_protocol_name for the TxnSystemClient

In clusters witha custom principal we need to set the
sasl_protocol_name on the embeded client within the
TxnSystemClient.

This logic matches that in server_base.cc for server
messenger configuration.

Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M src/kudu/transactions/txn_system_client.cc
2 files changed, 32 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/17480/1
--
To view, visit http://gerrit.cloudera.org:8080/17480
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic7b7c7fd154796215bd3a3ce748a2fd3155adebf
Gerrit-Change-Number: 17480
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Hongjiang Zhang (Code Review)
Hongjiang Zhang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@11
PS15, Line 11: If the kudu-master does not set "-enable_table_write_limit=true"
> How about:
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@12
PS15, Line 12: failed
> nit: fails
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@12
PS15, Line 12: cli
> nit: CLI
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@15
PS15, Line 15: cli
> nit: CLI
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@15
PS15, Line 15: Only kudu-master sets "-enable_table_write_limit=true"
> How about:
Ack



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 02:29:35 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Hongjiang Zhang (Code Review)
Hongjiang Zhang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2899
PS15, Line 2899: Alter
> nit: altering
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960
PS15, Line 2960: Modifying
> nit: Setting
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960
PS15, Line 2960: LOG(INFO)
> nit: if logging about setting a particular limit for a table disk size, why
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973
PS15, Line 2973: Modifying
> nit: Setting
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973
PS15, Line 2973: LOG(INFO)
> nit: if logging about setting a particular limit for a table row count limi
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@3975
PS15, Line 3975:   KuduSchemaBuilder schema_builder;
   :   schema_builder.AddColumn("key")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull()
   :   ->PrimaryKey();
   :   schema_builder.AddColumn("value")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull();
   :   KuduSchema schema;
   :   ASSERT_OK(schema_builder.Build());
   :
   :   // Create the table.
   :   TestWorkload workload(cluster_.get());
   :   workload.set_table_name(kTableName);
   :   workload.set_schema(schema);
   :   workload.set_num_replicas(1);
   :   workload.Setup();
> nit: To avoid extraneous test code, we don't need to define a schema since
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4043
PS15, Line 4043:   KuduSchemaBuilder schema_builder;
   :   schema_builder.AddColumn("key")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull()
   :   ->PrimaryKey();
   :   schema_builder.AddColumn("value")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull();
   :   KuduSchema schema;
   :   ASSERT_OK(schema_builder.Build());
> nit: same as above, it's not necessary to define the schema for the test ta
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4086
PS15, Line 4086:  master_addr, kTableName),
> Maybe, add the following test cases as well:
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@420
PS15, Line 420: disk_size_limit_str == "unlimited"
> nit here and below: maybe, compare this in a case-insensitive manner?  Ther
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@422
PS15, Line 422: safe_strtou64
> Here and below: why not to use safe_strto64() to read in a number as int64_
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@426
PS15, Line 426: disk_size_limit = u_disk_size_limit
> Here and below: disk_size_limit might end up being negative if a number gre
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@440
PS15, Line 440: row_count_limit_str == "unlimited"
> nit: maybe, compare this in a case-insensitive manner?  There is iequals()
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1220
PS15, Line 1220: disk_size_limit
> Why not just 'disk_size' ?  The upper context is already called 'set_limit'
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1227
PS15, Line 1227: row_count_limit
> Ditto: leave just 'row_count'?
Ack


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1234
PS15, Line 1234: of the table
> nit: for a table
Ack



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: 

[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17479 )

Change subject: [java] a property to show output while running a test
..


Patch Set 2:

> LGTM based on this documentation: 
> https://docs.gradle.org/current/dsl/org.gradle.api.tasks.testing.logging.TestLogging.html

Thank you for review!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Bankim Bhavsar 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 01:58:42 +
Gerrit-HasComments: No


[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/17479 )

Change subject: [java] a property to show output while running a test
..

[java] a property to show output while running a test

While working on non-trivial and relatively long running test scenarios,
I found it's inconvenient that the test output isn't shown while a test
scenario is running.  This patch introduces a new command-line property
'showTestOutput' to direct gradle outputting stdout and stderr from the
JVM on the console, so it's possible to do the standard stream
redirection and other stream manipulation in a command line shell.

An example of running a particular test scenario with the output
redirected to the console:

  ./gradlew -DshowTestOutput cleanTest :kudu-client:test --test ...

By default the property isn't set, so this patch preserves the original
behavior w.r.t. the output of running Java tests.

Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Reviewed-on: http://gerrit.cloudera.org:8080/17479
Tested-by: Kudu Jenkins
Reviewed-by: Bankim Bhavsar 
Reviewed-by: Andrew Wong 
---
M java/gradle/tests.gradle
1 file changed, 4 insertions(+), 0 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Bankim Bhavsar: Looks good to me, but someone else must approve
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Bankim Bhavsar 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job   Adding a couple of options of the Kudu Restore job:

2021-05-20 Thread Abhishek Chennaka (Code Review)
Abhishek Chennaka has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17388 )

Change subject: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore 
job   Adding a couple of options of the Kudu Restore job:
..


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG
Commit Message:

PS4:
> Same nit feedback as those around the Options.
Will do


http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@7
PS4, Line 7: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job  
   : Adding a couple of options of the Kudu Restore job:
> nit: put these on the same line
Will do


http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@7
PS4, Line 7: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job  
   : Adding a couple of options of the Kudu Restore job:
> I thought the first line was a summary, and the second was the start of the
Ah thanks for pointing that out. Will update the commit message


http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@11
PS4, Line 11: This will overwrite any existing
: database and if there is no existing database
> Kudu doesn't have a notion of a database of a standalone entity, so it's a
Right, Kudu doesn't have the notion of a database. But in here we are assuming 
full table name consists of an optional database name and a table 
name.(fulltablename=databasename.tablename). Maybe I can clarify that in the 
commit message.

Having a new table suffix or a new database name added would create a new table 
if the table is not present. In our case if we restore the table in the same 
cluster then we will be having both olddb.table and newdb.table with identical 
data. The backup/restore job will not affect the source tables.


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@57
PS4, Line 57: fullTableName.startsWith("impala::")
> nit: maybe store these in local variables; at least the ones that are reusa
Makes sense. Thanks for the review. Will update this in the next patch set


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@238
PS4, Line 238:   "If set, replaces the existing database name and if 
there is no existing database name, a new database name " +
> nit: maybe we should explicitly mention that if not set, or set to "", this
Will do


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@239
PS4, Line 239: For E
> nit: either "For example" or "E.g."
Noted


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240
PS4, Line 240: y
> nit: add a comma
Noted


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240
PS4, Line 240: be having
> nit: "will have"
Noted


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File 
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@801
PS4, Line 801: val options = createRestoreOptions(Seq(tableName))
> Do we have any remaining test coverage for if the new database isn't set? P
Yea we do not. Did this based on the how the table suffix test cases are 
implemented. Can reduce this down to one test case.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I65adcc1b3de0a8e1ac5b7f50a2d3a7036aa69421
Gerrit-Change-Number: 17388
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka 
Gerrit-Reviewer: Abhishek Chennaka 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 00:55:25 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@11
PS15, Line 11: If the kudu-master does not set "-enable_table_write_limit=true"
How about:

If '--enable_table_write_limit' is not set 'true' for kudu-master


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@12
PS15, Line 12: cli
nit: CLI


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@12
PS15, Line 12: failed
nit: fails


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@15
PS15, Line 15: Only kudu-master sets "-enable_table_write_limit=true"
How about:

Only when '--enable_table_write_limit' is set 'true'


http://gerrit.cloudera.org:8080/#/c/17444/15//COMMIT_MSG@15
PS15, Line 15: cli
nit: CLI



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 21 May 2021 00:02:24 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15: Code-Review+1

(14 comments)

Overall looks good!  Just a request to add a few more test cases and do minor 
changes in the names of CLI sub-commands.

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2899
PS15, Line 2899: Alter
nit: altering


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960
PS15, Line 2960: Modifying
nit: Setting


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960
PS15, Line 2960: LOG(INFO)
nit: if logging about setting a particular limit for a table disk size, why not 
to log about resetting the limit to the default setting as well?


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973
PS15, Line 2973: Modifying
nit: Setting


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973
PS15, Line 2973: LOG(INFO)
nit: if logging about setting a particular limit for a table row count limit, 
why not to log about resetting the limit to the default setting as well?


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4043
PS15, Line 4043:   KuduSchemaBuilder schema_builder;
   :   schema_builder.AddColumn("key")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull()
   :   ->PrimaryKey();
   :   schema_builder.AddColumn("value")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull();
   :   KuduSchema schema;
   :   ASSERT_OK(schema_builder.Build());
nit: same as above, it's not necessary to define the schema for the test table 
since TestWorkload has one by default.


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4086
PS15, Line 4086:  master_addr, kTableName),
Maybe, add the following test cases as well:
  * a negative value for the limit
  * the value greater than INT64_MAX


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@420
PS15, Line 420: disk_size_limit_str == "unlimited"
nit here and below: maybe, compare this in a case-insensitive manner?  There is 
iequals() function for that (see src/kudu/utils/string_case.h).


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@422
PS15, Line 422: safe_strtou64
Here and below: why not to use safe_strto64() to read in a number as int64_t?


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@426
PS15, Line 426: disk_size_limit = u_disk_size_limit
Here and below: disk_size_limit might end up being negative if a number greater 
than INT64_MAX is specified.  Maybe, read in the input above using 
safe_strto64() instead?


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@440
PS15, Line 440: row_count_limit_str == "unlimited"
nit: maybe, compare this in a case-insensitive manner?  There is iequals() 
function for that (see src/kudu/utils/string_case.h).


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1220
PS15, Line 1220: disk_size_limit
Why not just 'disk_size' ?  The upper context is already called 'set_limit', so 
duplicating 'limit' isn't necessary.


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1227
PS15, Line 1227: row_count_limit
Ditto: leave just 'row_count'?


http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1234
PS15, Line 1234: of the table
nit: for a table



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 23:37:18 +
Gerrit-HasComments: Yes


[kudu-CR] [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job   Adding a couple of options of the Kudu Restore job:

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17388 )

Change subject: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore 
job   Adding a couple of options of the Kudu Restore job:
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@7
PS4, Line 7: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job  
   : Adding a couple of options of the Kudu Restore job:
> nit: put these on the same line
I thought the first line was a summary, and the second was the start of the 
description.  We have best practices document linked from 
https://kudu.apache.org/docs/contributing.html#_submitting_patches : 
https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines


http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@11
PS4, Line 11: This will overwrite any existing
: database and if there is no existing database
Kudu doesn't have a notion of a database of a standalone entity, so it's a bit 
strange to read 'existing database'.

Also, from this description it's not quite clear what happens with the table 
olddb.table when --newDatabaseName=newdb: is the olddb.table will be dropped or 
it will stay along with the newly created newdb.table created by the restore 
job?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I65adcc1b3de0a8e1ac5b7f50a2d3a7036aa69421
Gerrit-Change-Number: 17388
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka 
Gerrit-Reviewer: Abhishek Chennaka 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 23:02:27 +
Gerrit-HasComments: Yes


[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17479 )

Change subject: [java] a property to show output while running a test
..


Patch Set 2: Code-Review+2

LGTM based on this documentation: 
https://docs.gradle.org/current/dsl/org.gradle.api.tasks.testing.logging.TestLogging.html


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Bankim Bhavsar 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 21:51:34 +
Gerrit-HasComments: No


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15: Code-Review+2

I'd actually be OK merging this as is, if there is no other feedback. Would be 
nice to get this into the 1.15 branch if we can.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 21:26:25 +
Gerrit-HasComments: No


[kudu-CR] KUDU-3223: management of per-table limit by kudu CLI

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17444 )

Change subject: KUDU-3223: management of per-table limit by kudu CLI
..


Patch Set 15: Code-Review+1

(2 comments)

LGTM, minus one small testing nit.

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@3975
PS15, Line 3975:   KuduSchemaBuilder schema_builder;
   :   schema_builder.AddColumn("key")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull()
   :   ->PrimaryKey();
   :   schema_builder.AddColumn("value")
   :   ->Type(client::KuduColumnSchema::INT32)
   :   ->NotNull();
   :   KuduSchema schema;
   :   ASSERT_OK(schema_builder.Build());
   :
   :   // Create the table.
   :   TestWorkload workload(cluster_.get());
   :   workload.set_table_name(kTableName);
   :   workload.set_schema(schema);
   :   workload.set_num_replicas(1);
   :   workload.Setup();
nit: To avoid extraneous test code, we don't need to define a schema since the 
TestWorkload has one by default. Some other tests do since their test coverage 
relies on having specific schemas and columns, but these new tests don't.


http://gerrit.cloudera.org:8080/#/c/17444/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/17444/7/src/kudu/tools/tool_action_table.cc@1210
PS7, Line 1210: table_creator->dimension_label(table_req.dimension_label());
  :   }
  :   return table_creator->Create();
  : }
  :
  :
  : } // anonymous
> BTW, what about using variadic arguments to read in the values?  Also, it's
Nice suggestion. This approach is much more elegant indeed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3
Gerrit-Change-Number: 17444
Gerrit-PatchSet: 15
Gerrit-Owner: Hongjiang Zhang 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hongjiang Zhang 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 21:21:02 +
Gerrit-HasComments: Yes


[kudu-CR] [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job   Adding a couple of options of the Kudu Restore job:

2021-05-20 Thread Andrew Wong (Code Review)
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17388 )

Change subject: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore 
job   Adding a couple of options of the Kudu Restore job:
..


Patch Set 4:

(8 comments)

Thanks for updating the behavior!

http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG
Commit Message:

PS4:
Same nit feedback as those around the Options.


http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@7
PS4, Line 7: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job  
   : Adding a couple of options of the Kudu Restore job:
nit: put these on the same line


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@57
PS4, Line 57: fullTableName.startsWith("impala::")
nit: maybe store these in local variables; at least the ones that are reusable. 
Then we can simplify the logic a bit to something like

E.g.

var databaseName = ""
var impalaPrefixLen = fullTableName.startsWith("impala::") ? 8 : 0
var indexOfPeriod = fullTableName.indexOf('.') // returns -1 if doesn't it 
exist; we can also use (indexOfPeriod == -1) instead of String.contains('.')
if (indexOfPeriod == -1) {
  databaseName = fullTableName.substr(impalaPrefixLen, indexOfPeriod)
  onlyTableName = fullTableName.substr(indexOfPeriod + 1)
} else {
  onlyTableName = fullTableName.substr(impalaPrefixLen)
}
// use the above variables to build the right restore name


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@238
PS4, Line 238:   "If set, replaces the existing database name and if 
there is no existing database name, a new database name " +
nit: maybe we should explicitly mention that if not set, or set to "", this 
doesn't change the database name? In case users think it removes the db name.


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@239
PS4, Line 239: For E
nit: either "For example" or "E.g."


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240
PS4, Line 240: y
nit: add a comma


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240
PS4, Line 240: be having
nit: "will have"


http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File 
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@801
PS4, Line 801: val options = createRestoreOptions(Seq(tableName))
Do we have any remaining test coverage for if the new database isn't set? 
Perhaps it's worth leaving the default empty and adding an explicit test for 
both adding and changing the database name?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I65adcc1b3de0a8e1ac5b7f50a2d3a7036aa69421
Gerrit-Change-Number: 17388
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka 
Gerrit-Reviewer: Abhishek Chennaka 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 21:00:22 +
Gerrit-HasComments: Yes


[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Bankim Bhavsar (Code Review)
Bankim Bhavsar has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17479 )

Change subject: [java] a property to show output while running a test
..


Patch Set 2: Code-Review+1

This looks very useful.
+1 because I'm not familiar with gradle and I'm not sure if there is a better 
way...


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Bankim Bhavsar 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 May 2021 20:09:18 +
Gerrit-HasComments: No


[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Alexey Serbin (Code Review)
Hello Kudu Jenkins, Grant Henke,

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

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

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

Change subject: [java] a property to show output while running a test
..

[java] a property to show output while running a test

While working on non-trivial and relatively long running test scenarios,
I found it's inconvenient that the test output isn't shown while a test
scenario is running.  This patch introduces a new command-line property
'showTestOutput' to direct gradle outputting stdout and stderr from the
JVM on the console, so it's possible to do the standard stream
redirection and other stream manipulation in a command line shell.

An example of running a particular test scenario with the output
redirected to the console:

  ./gradlew -DshowTestOutput cleanTest :kudu-client:test --test ...

By default the property isn't set, so this patch preserves the original
behavior w.r.t. the output of running Java tests.

Change-Id: I82590957004eef427a19f36a32e903012ea220d4
---
M java/gradle/tests.gradle
1 file changed, 4 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/17479/2
--
To view, visit http://gerrit.cloudera.org:8080/17479
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] [java] a property to show output while running a test

2021-05-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/17479


Change subject: [java] a property to show output while running a test
..

[java] a property to show output while running a test

While working on non-trivial and relatively long running test scenarios,
I found it's inconvenient that the test output isn't shown while a test
scenario is running.  This patch introduces a new command-line property
'testShowOutput' to direct gradle outputting stdout and stderr from the
JVM on the console, so it's possible to do the standard stream
redirection and other stream manipulation in a command line shell.

An example of running a particular test scenario with the output
redirected to the console:

  ./gradlew -DtestShowOutput cleanTest :kudu-client:test --test ...

By default the property isn't set, so this patch preserves the original
behavior w.r.t. the output of running Java tests.

Change-Id: I82590957004eef427a19f36a32e903012ea220d4
---
M java/gradle/tests.gradle
1 file changed, 4 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/17479/1
--
To view, visit http://gerrit.cloudera.org:8080/17479
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I82590957004eef427a19f36a32e903012ea220d4
Gerrit-Change-Number: 17479
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin