[kudu-CR] tool: port ts-cli

[Adar Dembo (Code Review)]

Hello Dinesh Bhat, Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tool: port ts-cli
..

tool: port ts-cli

I chose to expose common server functionality in new 'master' and 'tserver'
modes rather than consolidating them into a shared 'server' mode; I found
this to be more more intuitive.

Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
---
M docs/release_notes.adoc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/tablet/tablet_peer.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
A src/kudu/tools/tool_action_master.cc
A src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_main.cc
D src/kudu/tools/ts-cli.cc
17 files changed, 759 insertions(+), 521 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1301 - [python] Tests leak tmp directory

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1301 - [python] Tests leak tmp directory
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4372/1//COMMIT_MSG
Commit Message:

Line 11: change http://gerrit.cloudera.org:8080/#/c/1817/, the check for
nit: better to mention it by its commit hash (since in 5 years the gerrit 
server might have changed URLs, but git is forever)


Line 13: fixes the leak and re-enables the leak check for python.
does it also fix the process leak?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95d03ab1c23c7712ab647e37a646d5e22a758a9b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] linked list-test: set longer history retention

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: linked_list-test: set longer history retention
..


Patch Set 1: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/3356/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] tablet history gc-itest: set scanner ttl ms high

[Todd Lipcon (Code Review)]

Hello Mike Percy,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tablet_history_gc-itest: set scanner_ttl_ms high
..

tablet_history_gc-itest: set scanner_ttl_ms high

This test is currently flaky on slower builds (TSAN/ASAN) because of
scanner expiration. The test opens scanners and then waits some time
before actually reading from them. In a slow build, this might be more
than the default 1-minute scanner TTL, which would cause the scanner to
expire and the test to fail to read.

Change-Id: I4bd8a5bdc77871a95b3630f1622a802967d14b84
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 4 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4bd8a5bdc77871a95b3630f1622a802967d14b84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [python] Implement Scan Token API

[Jordan Birdsell (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [python] Implement Scan Token API
..

[python] Implement Scan Token API

First attempt at implementing the
[Scan Token API](http://gerrit.cloudera.org:8080/#/c/2443/) for the Python
client. This patch should also resolve KUDU-1401.  I included several
unit tests, most of which were based on the current scanner unit tests.

Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
---
M python/kudu/__init__.py
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
A python/kudu/tests/test_scantoken.py
M python/setup.py
5 files changed, 623 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/4367/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [python] Implement Scan Token API

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [python] Implement Scan Token API
..


Patch Set 7:

Build Started http://104.196.14.100/job/kudu-gerrit/3352/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] dist test: enable sharding on more long tests

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: dist_test: enable sharding on more long tests
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3361/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b860d584203c1b9b1ad472f620fe06c03eaaa21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] dist test: enable sharding on more long tests

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: dist_test: enable sharding on more long tests
..

dist_test: enable sharding on more long tests

This enables dist-test sharding for some more of the longest-running
tests.

Change-Id: I2b860d584203c1b9b1ad472f620fe06c03eaaa21
---
M build-support/dist_test.py
1 file changed, 13 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b860d584203c1b9b1ad472f620fe06c03eaaa21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

(38 comments)

http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:

PS8, Line 71: local_repica
local_replica


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

PS5, Line 517:   ASSERT_STR_MATCHES(stdout, "Header:");
 :   ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Fixed, it was a blind copy-paste effect.
Hmm, not exactly what I meant. What I mean is: you're already doing a 
substitute on L521; include the fs_wal_dir and fs_data_dirs substitution in 
there rather than doing it out here.

That way the entire command line is more clear; don't need to look in two 
different places.


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

Line 20: #include 
Nit: should precede string.


Line 215: const vector kLocalReplicaModeRegexes = {
Shouldn't there be a dump mode in here? And something for "list all replicas"?


PS8, Line 594:   string fs_paths = "--fs_wal_dir=" + kTestDir + " "
 :   "--fs_data_dirs=" + kTestDir;
Same comment about partial substitution here.


Line 596:   LOG(INFO) <

[kudu-CR] tool: port ts-cli

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: port ts-cli
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3354/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port ts-cli

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: tool: port ts-cli
..

tool: port ts-cli

I chose to expose common server functionality in new 'master' and 'tserver'
modes rather than consolidating them into a shared 'server' mode; I found
this to be more more intuitive.

Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
---
M build-support/dist_test.py
M docs/release_notes.adoc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/tablet/tablet_peer.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
A src/kudu/tools/tool_action_master.cc
A src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_main.cc
D src/kudu/tools/ts-cli.cc
18 files changed, 759 insertions(+), 522 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1301 - [python] Tests leak tmp directory

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1301 - [python] Tests leak tmp directory
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3351/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95d03ab1c23c7712ab647e37a646d5e22a758a9b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1301 - [python] Tests leak tmp directory

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1301 - [python] Tests leak tmp directory
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3358/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95d03ab1c23c7712ab647e37a646d5e22a758a9b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1301 - [python] Tests leak tmp directory

[Jordan Birdsell (Code Review)]

Jordan Birdsell has posted comments on this change.

Change subject: KUDU-1301 - [python] Tests leak tmp directory
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4372/1//COMMIT_MSG
Commit Message:

Line 11: commit 89380b53d48b085dfe394b9966360d1ef1a4b038, the check for
> nit: better to mention it by its commit hash (since in 5 years the gerrit s
Done


Line 13: fixes both the temp directory and the process leak and re-enables
> does it also fix the process leak?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95d03ab1c23c7712ab647e37a646d5e22a758a9b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] linked list-test: set longer history retention

[Todd Lipcon (Code Review)]

Hello Mike Percy,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: linked_list-test: set longer history retention
..

linked_list-test: set longer history retention

The default 15 minute history retention was causing linked_list-test to
GC some history in the case that -seconds_to_run was set significantly
longer than the default.

This sets the retention to 1 day to prevent such issues.

I also removed an old unused function I noticed while making this
change.

Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
---
M src/kudu/integration-tests/linked_list-test.cc
1 file changed, 4 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] linked list-test: set longer history retention

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: linked_list-test: set longer history retention
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3355/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] python: update cython version check in setup.py

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: python: update cython version check in setup.py
..


python: update cython version check in setup.py

This updates the required version to match requirements.txt

Change-Id: Id44a716a6917bfdc37a96ba4759cc007805285a5
Reviewed-on: http://gerrit.cloudera.org:8080/4368
Reviewed-by: Jordan Birdsell 
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M python/setup.py
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Jordan Birdsell: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id44a716a6917bfdc37a96ba4759cc007805285a5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: port ts-cli

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: tool: port ts-cli
..


Patch Set 2:

(5 comments)

I agree from the user perspective it's nice to expose the generic stuff in both 
'master' and 'tserver', but wonder if we could share slightly more code between 
them. Could you reuse the same 'action' in both places?

http://gerrit.cloudera.org:8080/#/c/4373/2/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 53: DECLARE_int64(timeout_ms); // defined in ksck
maybe move to 'common' since it's a "common" flag?


Line 55: DEFINE_bool(force, false, "If true, allows the set_flag command to set 
a flag "
I foresee a future issue here: I bet we'll have other flags with '--force' 
arguments, and we're going to need to have per-subtool overrides of this help 
text. No action here yet but just noticing that our yak is looking a little 
hairy and may need some shaving soon.


Line 213:   MessengerBuilder builder("kudu");
odd name


Line 214:   RETURN_NOT_OK(builder.Build());
perhaps just define the builder inline here? 
MessengerBuilder("tool").Build()


http://gerrit.cloudera.org:8080/#/c/4373/2/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

PS2, Line 44: typename T
I think 'class ProxyClass' would be clearer (I usually only use 'typename' if 
it is expected to plausibly be a primitive)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] python: bump version number to 0.3.0

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: python: bump version number to 0.3.0
..


python: bump version number to 0.3.0

There have been some changes to the Python client since our last ASF
release, so we need to bump the version number before we do our next ASF
release.

Change-Id: I8c874f9f9fb235b979e3120304d52d59061a26d6
Reviewed-on: http://gerrit.cloudera.org:8080/4364
Tested-by: Kudu Jenkins
Reviewed-by: Jordan Birdsell 
Reviewed-by: David Ribeiro Alves 
---
M python/setup.py
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Jordan Birdsell: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8c874f9f9fb235b979e3120304d52d59061a26d6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] flex partitioning-itest: shard into separate cases

[Todd Lipcon (Code Review)]

Hello Dan Burkert,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: flex_partitioning-itest: shard into separate cases
..

flex_partitioning-itest: shard into separate cases

Rather than doing a loop in the main test case, shard the test into one
test case per setup.

This slightly increases the runtime of a non-parallel invocation, since
we now build and tear down a cluster for each configuration. However,
this permits parallelizing its run on dist-test. Since this is one of
the longer-running tests, it's worth it.

Change-Id: I6d352be66573fb95aac1e3ee8bc74569d4e8a0b3
---
M src/kudu/integration-tests/flex_partitioning-itest.cc
1 file changed, 58 insertions(+), 41 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6d352be66573fb95aac1e3ee8bc74569d4e8a0b3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..


Patch Set 7:

(1 comment)

Thanks, please take a look at updated patch.

http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 63
> I'd prefer option #2, as it strips away more of the existing code and (hope
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [python] Implement Scan Token API

[Jordan Birdsell (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [python] Implement Scan Token API
..

[python] Implement Scan Token API

First attempt at implementing the
[Scan Token API](http://gerrit.cloudera.org:8080/#/c/2443/) for the Python
client. This patch should also resolve KUDU-1401.  I included several
unit tests, most of which were based on the current scanner unit tests.

Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
---
M python/kudu/__init__.py
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
A python/kudu/tests/test_scantoken.py
M python/setup.py
5 files changed, 627 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/4367/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [python] Implement Scan Token API

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [python] Implement Scan Token API
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4367/5/python/kudu/tests/test_scantoken.py
File python/kudu/tests/test_scantoken.py:

Line 35: Stollen from the the test scanner given the simiilarity in
Stolen & Similarity are mispelled


Line 77: input = [(token.serialize(), self.master_host, 
self.master_port) for token in builder.build()]
It looks like this line and below is repeated for the three tests, could you 
roll this into _get_scan_token_results to help remove some of the duplication?


http://gerrit.cloudera.org:8080/#/c/4367/5/python/requirements.txt
File python/requirements.txt:

Line 6: multiprocessing
This is a test-only dependency, right?  Is there anyway to express that with 
pip?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [python] Implement Scan Token API

[Jordan Birdsell (Code Review)]

Jordan Birdsell has posted comments on this change.

Change subject: [python] Implement Scan Token API
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4367/5/python/kudu/tests/test_scantoken.py
File python/kudu/tests/test_scantoken.py:

Line 35: Stolen from the the test scanner given the similarity in
> Stolen & Similarity are mispelled
Done


Line 77: builder = self.table.scan_token_builder()
> It looks like this line and below is repeated for the three tests, could yo
Cant move it to _get_scan_token_results since I need to serialize the tokens in 
the main thread before shipping to the new threads. However, I did create a 
helper function within the class to clean this up a bit.


http://gerrit.cloudera.org:8080/#/c/4367/5/python/requirements.txt
File python/requirements.txt:

Line 6
> This is a test-only dependency, right?  Is there anyway to express that wit
Good point, i moved this to setup.py in the tests_require list.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I710c93e51ab5f0f5ed038aaaf1925b58c576b655
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 854 insertions(+), 805 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: [java] - Synchronize the removal of the TabletClient from 
client2tablets and ip2client
..


Patch Set 1: Verified+1

unrelated failure org.apache.kudu.client.ITClient.test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add RegexpKuduOperationsProducer class

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Add RegexpKuduOperationsProducer class
..


Patch Set 5:

> Late thought: do we want this in 1.0? If so will make note to Todd
 > to add to his release note rework patch once this one merged (or
 > will add myself if Todd's gets merged first)

I'm OK with it going into 1.0. What you can do is checkout his doc patch using 
the Gerrit "download" link, add a patch on top of it, then submit to Gerrit. 
Gerrit will keep track of the dependency.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd64542095f0064a21635ec76c46d6a1be98b7ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Ara Ebrahimi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Add RegexpKuduOperationsProducer class

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Add RegexpKuduOperationsProducer class
..


Add RegexpKuduOperationsProducer class

This patch adds the RegexpKuduOperationsProducer class. This class
serializes Event objects to Kudu inserts or upserts by decoding
the body into a string, parsing the string using a regular
expression, and finally mapping match groups to columns by
matching the name of the match group to the name of the column.
Parsed values are naively coerced to the proper type.

This provides an easy-to-use but flexible way to ingest data with
varying schemas into Kudu from Flume.

Change-Id: Ibd64542095f0064a21635ec76c46d6a1be98b7ea
Reviewed-on: http://gerrit.cloudera.org:8080/3883
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
A 
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java
A 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java
2 files changed, 498 insertions(+), 0 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibd64542095f0064a21635ec76c46d6a1be98b7ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Ara Ebrahimi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Add RegexpKuduOperationsProducer class

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Add RegexpKuduOperationsProducer class
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd64542095f0064a21635ec76c46d6a1be98b7ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Ara Ebrahimi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] WIP: script to run clang-tidy against a patch

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: WIP: script to run clang-tidy against a patch
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3366/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c6246f4691b0ca143eea4aa8fb6a4b23099ce4e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] linked list-test: set longer history retention

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: linked_list-test: set longer history retention
..


linked_list-test: set longer history retention

The default 15 minute history retention was causing linked_list-test to
GC some history in the case that -seconds_to_run was set significantly
longer than the default.

This sets the retention to 1 day to prevent such issues.

I also removed an old unused function I noticed while making this
change.

Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
Reviewed-on: http://gerrit.cloudera.org:8080/4374
Reviewed-by: Mike Percy 
Tested-by: Mike Percy 
---
M src/kudu/integration-tests/linked_list-test.cc
1 file changed, 4 insertions(+), 10 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I43395eb73c99f79be1884b6a22fab06f22c58980
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/3362/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 806 insertions(+), 817 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/9
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

(35 comments)

Thank you again Adar, addressed rev comments below, please see responses too 
inline for few of the comments.

http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:

PS8, Line 71: local_repica
> local_replica
Done


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

PS5, Line 517:   ASSERT_STR_MATCHES(stdout, "Header:");
 :   ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Hmm, not exactly what I meant. What I mean is: you're already doing a subst
I see, sorry for misunderstanding earlier comment. Given that these are common 
args for almost all commands I liked defining them in one place with an 
intuitive variable name instead of spraying "--" args everywhere. I am keeping 
them as-is where they are used multiple times but only changing them at places 
where they are used only once. Lemme know.


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

Line 20: #include 
> Nit: should precede string.
Done


Line 215: const vector kLocalReplicaModeRegexes = {
> Shouldn't there be a dump mode in here? And something for "list all replica
Done


PS8, Line 594:   string fs_paths = "--fs_wal_dir=" + kTestDir + " "
 :   "--fs_data_dirs=" + kTestDir;
> Same comment about partial substitution here.
Done


Line 596:   LOG(INFO) < This was to help you debug, right? Can it be removed now?
thank you, good catch.


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

Line 19: #include "kudu/tools/tool_action_common.h"
> As before, this include doesn't belong up here.
Done


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 28: #include "kudu/common/schema.h"
> Nit: should go after row*
Done


Line 33: #include "kudu/consensus/consensus.pb.h"
> Nit: shoudl go after consensus_meta.h.
Done


Line 53: #include "kudu/tablet/rowset_metadata.h"
> Nit: should come before tablet.h.
Done


Line 55: #include "kudu/tserver/tserver.pb.h"
> Nit: should go after tablet_copy_client.h.
Done


PS8, Line 58: #include "kudu/util/logging.h"
> Nit: should go after env_util.h.
Thank you, no doubt I did a pretty bad job here :)


PS8, Line 71: information(if any)
> Nit: information (if any)
Done


PS8, Line 114: DumpOptions
> I'd drop this struct altogether, because:
Agreed, done.


PS8, Line 286: static
> Nit: don't need this; the function is already in an anonymous namespace.
Fixed.


Line 287:   const RowSetMetadata& rs_meta) {
> Nit: fix the indentation on this line.
Done


Line 292: std::cout << "Column block for column ID " << col_id;
> std::cout and std::endl are already in the 'using' blocks above, so you can
This became one  indentation adjustment fun in the entire file :)


PS8, Line 318:   const string* tablet_id = FindOrNull(context.required_args, 
"tablet_id");
 :   if (tablet_id == nullptr) {
 : LOG(INFO) << "No tablet_id specified, dumping all tablets:";
 :   }
> I understand the existing tool allowed this 'no tablet_id means dump all ta
Good catch, fixed.


PS8, Line 346: FsManager& fs_manager
> Google style frowns on passing arguments by ref. Your options are:
Cool, thanks. I changed them to pointer - pointer mainly because few following 
callsites like FsManager::OpenBlock has non-const nature and also 
TabletMetadata::Load takes a raw pointer, etc. I didn't really chase after why 
Load function is taking a raw pointer to keep the scope of this change.


Line 381:   std::cout << "\t" << tablet << std::endl;
> In this case, let's not bother with the leading tab. It'd be easier to pars
Done


PS8, Line 398: scoped_refptr(nullptr)
> I think this can just be "scoped_refptr()".
Interesting, done. I didn't really take this as an opportunity to tamper with 
the original code, although part of the exercise was that.


PS8, Line 399: nullptr
> When passing a nullptr like this, consider the following style:
Very helpful, thank you.


PS8, Line 432: FsManager& fs_manager
> Const ref here too.
Done


PS8, Line 476:   // NewDeltaIterator returns Status::OK() iff a new 
DeltaIterator is created. Thus,
 :   // it's safe to have a gscoped_ptr take possesion of 
'raw_iter' here.
 :   gscoped_ptr delta_iter(raw_iter);
> This is correct, but let's use unique_ptr now; this was written before we t
Done


PS8, Line 540: FsManager& fs_manager
> Let's change this to const ref.
Couldn't because of the reason mentioned above, eventually they call into 

[kudu-CR] util: avoid redundant copies in various macros

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: util: avoid redundant copies in various macros
..


util: avoid redundant copies in various macros

A lot of macros have the form:

  #define FOO(arg) \
Foo arg_ = (arg); \
... do something with arg_ ...

Of course the purpose of the 'arg_' local is to avoid double-evaluation
of the argument in the case that it's something expensive or something
with a side effect. However, this pattern results in an extra copy of
'arg' in the case that it's actually a local, eg in the pattern:

  Foo f = ...;
  FOO(f);

This expands out to a redundant copy 'Foo arg_ = f;'.

It turns out that the solution is to make 'Foo arg_' a const reference
'const Foo& arg_'. At first, this seems wrong: references to a temporary
are often bad. However, it turns out that C++ has a rule that const
references to temporaries on the stack keep that temporary alive as long as
the reference stays in scope. So, this is safe, and avoids an extra
copy.

Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Reviewed-on: http://gerrit.cloudera.org:8080/4379
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M src/kudu/util/status.h
M src/kudu/util/test_macros.h
2 files changed, 13 insertions(+), 13 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Reorganize and fill out release notes for 1.0

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Reorganize and fill out release notes for 1.0
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4369/1//COMMIT_MSG
Commit Message:

Line 10: - removed verbiage about 'beta', since we are hitting 1.0
> Can this all this beta cleanup stuff be done in one, separate patch? For ex
The FAQ is a different branch though


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f35c3284d384b88be4581bed5f952e71c4af869
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1604 - [python] Fix bug getting table column by index

[Jordan Birdsell (Code Review)]

Jordan Birdsell has uploaded a new change for review.

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

Change subject: KUDU-1604 - [python] Fix bug getting table column by index
..

KUDU-1604 - [python] Fix bug getting table column by index

There is currently a bug when attempting to access a table column
by its index. When attempted, the column definition returned has
the index value as the column name. This patch fixes this bug so
that the appropriate column name is set and returned. An existing
test has been updated to account for this usage.

Change-Id: If754ddb463223430c7fe3c8920e433031162562e
---
M python/kudu/client.pyx
M python/kudu/tests/test_client.py
2 files changed, 11 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If754ddb463223430c7fe3c8920e433031162562e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 

[kudu-CR] KUDU-1604 - [python] Fix bug getting table column by index

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1604 - [python] Fix bug getting table column by index
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3363/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If754ddb463223430c7fe3c8920e433031162562e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] util: avoid redundant copies in various macros

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Mike Percy, Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: util: avoid redundant copies in various macros
..

util: avoid redundant copies in various macros

A lot of macros have the form:

  #define FOO(arg) \
Foo arg_ = (arg); \
... do something with arg_ ...

Of course the purpose of the 'arg_' local is to avoid double-evaluation
of the argument in the case that it's something expensive or something
with a side effect. However, this pattern results in an extra copy of
'arg' in the case that it's actually a local, eg in the pattern:

  Foo f = ...;
  FOO(f);

This expands out to a redundant copy 'Foo arg_ = f;'.

It turns out that the solution is to make 'Foo arg_' a const reference
'const Foo& arg_'. At first, this seems wrong: references to a temporary
are often bad. However, it turns out that C++ has a rule that const
references to temporaries on the stack keep that temporary alive as long as
the reference stays in scope. So, this is safe, and avoids an extra
copy.

Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
---
M src/kudu/util/status.h
M src/kudu/util/test_macros.h
2 files changed, 13 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] WIP: script to run clang-tidy against a patch

[Todd Lipcon (Code Review)]

Todd Lipcon has uploaded a new change for review.

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

Change subject: WIP: script to run clang-tidy against a patch
..

WIP: script to run clang-tidy against a patch

This is some WIP tooling which runs clang-tidy against a given patch,
and formats the resulting warnings in a form that can be consumed by the
gerrit API[1]

I'm hoping that, with a bit of tweaking, we can get this to be a useful
first-pass review to handle a lot of the more 'mechanical' things we
check.

Note if you want to try this you need clang-tidy on your path

[1] 
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-review

Change-Id: I6c6246f4691b0ca143eea4aa8fb6a4b23099ce4e
---
A build-support/clang_tidy_gerrit.py
A src/kudu/.clang-tidy
2 files changed, 120 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6c6246f4691b0ca143eea4aa8fb6a4b23099ce4e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 

[kudu-CR] util: avoid redundant copies in various macros

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: util: avoid redundant copies in various macros
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3364/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: WIP: [java] - Synchronize the removal of the TabletClient from 
client2tablets and ip2client
..

WIP: [java] - Synchronize the removal of the TabletClient from client2tablets 
and ip2client

When entries are added to the maps they are added under a synchronized block, 
but
since entries where never removed from client2tablets the removal was never 
synchronized.
This makes it so that we remove the entries from both maps under the same lock 
we use
to add them.

Change-Id: I86197aed50be52588c3debe590d660c709cddacd
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 13 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 

[kudu-CR] WIP: [java] - Synchronize the removal of the TabletClient from client2tablets and ip2client

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: WIP: [java] - Synchronize the removal of the TabletClient from 
client2tablets and ip2client
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3365/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86197aed50be52588c3debe590d660c709cddacd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] python: update cython version check in setup.py

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: python: update cython version check in setup.py
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id44a716a6917bfdc37a96ba4759cc007805285a5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] python: bump version number to 0.3.0

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: python: bump version number to 0.3.0
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c874f9f9fb235b979e3120304d52d59061a26d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No