[kudu-CR] WIP [client] expose non-voter replicas for kudu CLI tool

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8586 )

Change subject: WIP [client] expose non-voter replicas for kudu CLI tool
..


Patch Set 1:

(4 comments)

Looks alright to me until we can figure out how we want to expose this in the 
public client API

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.h@372
PS1, Line 372: bool show_non_voters = false
perhaps make this an enum? SHOW_NON_VOTERS / HIDE_NON_VOTERS

Also this doesn't need to be explicit and it's probably best to get rid of the 
default value in the constructor


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck_remote.cc@318
PS1, Line 318: show_non_voters_
might be a little friendlier to make this a setter method


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

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/tool_action_table.cc@64
PS1, Line 64: show_non_voters_
it would be less hacky to use a setter on Data for this


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/tool_action_table.cc@87
PS1, Line 87: is_voter_
mind putting a getter on this?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19317fdf5a2d5c8bb5f37b27bb83067a4df4ea20
Gerrit-Change-Number: 8586
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 17 Nov 2017 08:55:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2218. tls socket: properly handle temporary socket errors in Writev

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8570 )

Change subject: KUDU-2218. tls_socket: properly handle temporary socket errors 
in Writev
..


Patch Set 3:

> Change has been successfully cherry-picked as 
> 64eb9f37b171419ed12a3795efe28faf2fd33b3d
 > by Alexey Serbin

BTW, removing the SSL_MODE_ENABLE_PARTIAL_WRITE from TLS context options does 
not affect the test if running on OS X.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Gerrit-Change-Number: 8570
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 17 Nov 2017 18:25:54 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2218. tls socket: properly handle temporary socket errors in Writev

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8570 )

Change subject: KUDU-2218. tls_socket: properly handle temporary socket errors 
in Writev
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8570/2/src/kudu/security/tls_socket-test.cc
File src/kudu/security/tls_socket-test.cc:

http://gerrit.cloudera.org:8080/#/c/8570/2/src/kudu/security/tls_socket-test.cc@20
PS2, Line 20: #include 
nit: please move this into the section of C++ headers.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Gerrit-Change-Number: 8570
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 17 Nov 2017 17:42:11 +
Gerrit-HasComments: Yes

[kudu-CR] tablet: make various update paths atomic

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Mike Percy, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

Change subject: tablet: make various update paths atomic
..

tablet: make various update paths atomic

A few codepaths in the tablet subsystem aren't atomic, i.e. if they fail
mid-method (e.g. due to a file error), they leave some in-memory
structures updated and others untouched. This has been safe because we
CHECKed to ensure their success. In preparation for _not_ crashing in
these methods, this patch refactors some of these functions to be
atomic, and notes others that still have the possibility of failing in
such a state (these calls still trigger a CHECK failure).

There are no functional changes in this patch.

Change-Id: I0241cd4206ce77ef1fb334458b091bc2092f4141
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/metadata-test.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
9 files changed, 160 insertions(+), 149 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/8441/5
--
To view, visit http://gerrit.cloudera.org:8080/8441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0241cd4206ce77ef1fb334458b091bc2092f4141
Gerrit-Change-Number: 8441
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2218. tls socket: properly handle temporary socket errors in Writev

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8570 )

Change subject: KUDU-2218. tls_socket: properly handle temporary socket errors 
in Writev
..

KUDU-2218. tls_socket: properly handle temporary socket errors in Writev

This fixes a bug which caused RaftConsensusITest.TestLargeBatches to
fail when run under stress, as in the following command line:

taskset -c 0-4 \
 build/latest/bin/raft_consensus-itest \
   --gtest_filter=\*LargeBat\* \
   --stress-cpu-threads=8

This would produce an error like:
Network error: failed to write to TLS socket: error:1409F07F:SSL 
routines:SSL3_WRITE_PENDING:bad write retry:s3_pkt.c:878

This means that we were retrying a write after getting EAGAIN, but with
a different buffer than the first time.

I tracked this down to mishandling of temporary socket errors in
TlsSocket::Writev(). In the case that we successfully write part of the
io vector but hit such an error trying to write a later element in the
vector, we were still propagating the error back up to the caller. The
caller didn't realize that part of the write was successful, and thus it
would retry the write from the beginning.

The fix is to fix the above, but also to enable partial writes in
TlsContext. The new test fails if either of the above two changes are
backed out.

Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Reviewed-on: http://gerrit.cloudera.org:8080/8570
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
3 files changed, 219 insertions(+), 70 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Gerrit-Change-Number: 8570
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 

[kudu-CR] tablet: make various update paths atomic

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8441 )

Change subject: tablet: make various update paths atomic
..


Patch Set 5:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/8441/4//COMMIT_MSG@9
PS4, Line 9: he tablet
> nit: This seems like more of a requirement than an invariant to me. An inva
Ack


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/delta_compaction.cc
File src/kudu/tablet/delta_compaction.cc:

http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/delta_compaction.cc@372
PS4, Line 372:
> Please add a high level comment to this function regarding when we're allow
Done


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/delta_compaction.cc@386
PS4, Line 386: RETURN_NOT_OK(tracker->OpenDeltaReaders(new_undo_blocks, 
_undo_stores, UNDO));
> Add:
Done


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/delta_compaction.cc@388
PS4, Line 388:
> I'm curious if the order in which we update the stores matters. If so, it d
I don't think so given this all happens under locks.


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/delta_compaction.cc@390
PS4, Line 390:
> insert: for UNDOs
Done


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/diskrowset.cc@a577
PS4, Line 577:
 :
 :
> why was this comment removed?
Done


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/diskrowset.cc@567
PS4, Line 567:
> what does out of sync mean?
Done


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/diskrowset.cc@569
PS4, Line 569:
> By whom? What kind of guarantee do we get?
Took this out and replaced with CHECKs.


http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/tablet_mm_ops.cc
File src/kudu/tablet/tablet_mm_ops.cc:

http://gerrit.cloudera.org:8080/#/c/8441/4/src/kudu/tablet/tablet_mm_ops.cc@264
PS4, Line 264: oped_refptr 
MajorDeltaCompactionOp::RunningGauge() const {
 :   return tablet_->metrics()->delta_major_compact_rs_running;
 : }
 :
 : 
 : //
> This logic is reasonable up at this high-level driver code but it should be
Ack



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0241cd4206ce77ef1fb334458b091bc2092f4141
Gerrit-Change-Number: 8441
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 17:45:53 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Anonymous Coward (Code Review)]

Hello Chris George, Dan Burkert, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..

KUDU-1737 : Submit column characteristics via KuduContext

Submit defaultValue, desiredBlockSize, encoding and
compressionAlgorithm via metadata in schema when using KuduContext's
createTable.

Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
A 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextWithMetadataTest.scala
2 files changed, 87 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 2
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] dist test: enable sharding of a few more tests

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8565 )

Change subject: dist_test: enable sharding of a few more tests
..

dist_test: enable sharding of a few more tests

Adds sharding for rowset_tree-test, tablet_copy-itest, and
delete_tablet-itest which are the longest-running non-sharded tests in
recent builds such as [1]

[1] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1510801703.18407

Change-Id: Iff1e0ac39f1834e8ac22283e1dc0c336d328ae35
Reviewed-on: http://gerrit.cloudera.org:8080/8565
Reviewed-by: Dan Burkert 
Tested-by: Todd Lipcon 
---
M build-support/dist_test.py
1 file changed, 4 insertions(+), 1 deletion(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Todd Lipcon: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iff1e0ac39f1834e8ac22283e1dc0c336d328ae35
Gerrit-Change-Number: 8565
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] dist test: enable sharding of a few more tests

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8565 )

Change subject: dist_test: enable sharding of a few more tests
..


Patch Set 2: Verified+1

Unrelated test flakiness issues w/ infra


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff1e0ac39f1834e8ac22283e1dc0c336d328ae35
Gerrit-Change-Number: 8565
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 21:26:08 +
Gerrit-HasComments: No

[kudu-CR] dist test: enable sharding of a few more tests

[Todd Lipcon (Code Review)]

Todd Lipcon has removed a vote on this change.

Change subject: dist_test: enable sharding of a few more tests
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8565
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iff1e0ac39f1834e8ac22283e1dc0c336d328ae35
Gerrit-Change-Number: 8565
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] schema: micro-optimize num columns()

[Todd Lipcon (Code Review)]

Hello Alexey Serbin,

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

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

to review the following change.


Change subject: schema: micro-optimize num_columns()
..

schema: micro-optimize num_columns()

Looking at some profile earlier this week I noticed some 'imul'
instructions showing up where I didn't expect them. I traced this back
to calls to schema.num_columns() which I assumed was a simple load.
Rather, it turns out that vector::size() does 'vector.end() -
vector.start()', which has to divide by the size of the element in order
to calculate the size.

Since ColumnSchema has a somewhat odd size (72 bytes at the time of
writing), the division-by-72 gets implemented by a multiplication of its
inverse, rather than something cheaper like a bitshift.

Simpler, though, is to just use a different mechanism for
Schema::num_columns() which doesn't involve any calculation.

Didn't benchmark this since I'm not sure which benchmark might highlight
the change, but looking at generated assembly of CopyRow in release mode
shows the difference. Before, the loop condition looked like:

   0x01a2f0f8 <+328>:   add$0x1,%r15
   0x01a2f0fc <+332>:   mov%r12,%rdi
   0x01a2f0ff <+335>:   mov(%rdi),%rax
   0x01a2f102 <+338>:   mov0x8(%rax),%rcx
   0x01a2f106 <+342>:   sub(%rax),%rcx
   0x01a2f109 <+345>:   sar$0x5,%rcx
   0x01a2f10d <+349>:   movabs $0xaaab,%rdx
   0x01a2f117 <+359>:   imul   %rdx,%rcx
   0x01a2f11b <+363>:   movabs $0x1,%rdx
   0x01a2f125 <+373>:   add%rdx,%r14
   0x01a2f128 <+376>:   add$0xc,%r13
   0x01a2f12c <+380>:   cmp%r15,%rcx
   0x01a2f12f <+383>:   ja 0x1a2f000 
(kudu::RowBlockRow const&, kudu::ContiguousRow*, kudu::Arena*)+80>

After:

   0x01a2ea78 <+328>:   add$0x1,%r15
   0x01a2ea7c <+332>:   mov%r12,%rdi
   0x01a2ea7f <+335>:   mov(%rdi),%rax
   0x01a2ea82 <+338>:   movabs $0x1,%rcx
   0x01a2ea8c <+348>:   add%rcx,%r14
   0x01a2ea8f <+351>:   add$0xc,%r13
   0x01a2ea93 <+355>:   cmp%r15,0x80(%rax)
   0x01a2ea9a <+362>:   ja 0x1a2e980 
(kudu::RowBlockRow const&, kudu::ContiguousRow*, kudu::Arena*)+80>

Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d
---
M src/kudu/common/schema.h
1 file changed, 10 insertions(+), 4 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d
Gerrit-Change-Number: 8587
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 

[kudu-CR] [spark] Remove AsyncClient in KuduContext

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8551 )

Change subject: [spark] Remove AsyncClient in KuduContext
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8551/1//COMMIT_MSG@7
PS1, Line 7: [spark] Remove AsyncClient in KuduContext
> I suggested the removal, since it's not used internally, and both this API
Can we not just update both the sync and async clients? And propagate the max 
from either one?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8745d8503a35e17a632d0a0cde5738915f00fb
Gerrit-Change-Number: 8551
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 21:40:09 +
Gerrit-HasComments: Yes

[kudu-CR] catalog manager tsk-itest: ensure that test eventually makes progress

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8567 )

Change subject: catalog_manager_tsk-itest: ensure that test eventually makes 
progress
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8567/1/src/kudu/integration-tests/catalog_manager_tsk-itest.cc
File src/kudu/integration-tests/catalog_manager_tsk-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8567/1/src/kudu/integration-tests/catalog_manager_tsk-itest.cc@18
PS1, Line 18: #include 
> nit: maybe, replace with  and put along with the rest of C++ heade
Done


http://gerrit.cloudera.org:8080/#/c/8567/1/src/kudu/integration-tests/catalog_manager_tsk-itest.cc@20
PS1, Line 20: #include 
> warning: #includes are not sorted properly [llvm-include-order]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
Gerrit-Change-Number: 8567
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 21:48:39 +
Gerrit-HasComments: Yes

[kudu-CR] catalog manager tsk-itest: ensure that test eventually makes progress

[Todd Lipcon (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: catalog_manager_tsk-itest: ensure that test eventually makes 
progress
..

catalog_manager_tsk-itest: ensure that test eventually makes progress

This test previously tried to introduce a lot of master leader elections
by setting a very low heartbeat and failure interval. This worked, but
sometimes worked so well that the test never made progress and couldn't
obtain a stable leader long enough to create a table.

This patch changes the test to instead use a separate thread which
triggers elections manually on all the leaders. The elections start off
very frequent and then back off as the test progresses to ensure that by
the end, the leaders do actually make progress.

I verified that this still covers the case of a failed write when
writing TSKs by changing the RETURN_NOT_OK to a CHECK_OK when storing
the TSK. With the CHECK_OK, the test failed nearly immediately.

Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
---
M src/kudu/integration-tests/catalog_manager_tsk-itest.cc
1 file changed, 46 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
Gerrit-Change-Number: 8567
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP [catalog manager] introduce replica type matching policy

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: WIP [catalog manager] introduce replica type matching policy
..

WIP [catalog manager] introduce replica type matching policy

Introduced replica type matching policy for GetTabletLocations RPC and
various wrapper methods used in master, catalog manager, and tests.
The policy is used to specify what type of tablet replicas to locate
while populating the result of the GetTabletLocations RPC.

When not specified otherwise or when using earlier versions of the
GetTable[t]LocationsRequestPB protobuf messages, VOTER_REPLICA policy
is used.  To locate all replicas, specify ANY_REPLICA policy in the
match_policy field of the GetTable[t]LocationsRequestPB.  The latter
is done now unconditionally to enable already existing non-voter
replica-related tests pass.  This will change by a follow-up commit:
the 'regular' client operations should work with voter replicas only,
so the use cases for getting all tablet replicas will be just
the following:

  * the kudu CLI tool fetching that information to report on replica
membership status for listings in 'table list --tablets' and
'ksck' sub-commands.

  * various tests exercising the functionality related to
non-voter tablet replicas

WIP: perhaps, add more tests

Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
---
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
13 files changed, 164 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/8161/5
--
To view, visit http://gerrit.cloudera.org:8080/8161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
Gerrit-Change-Number: 8161
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] wire protocol: optimize RewriteRowBlockPointers

[Todd Lipcon (Code Review)]

Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: wire_protocol: optimize RewriteRowBlockPointers
..

wire_protocol: optimize RewriteRowBlockPointers

I was looking at why our tpch1 benchmark performance seems to have been
creeping up over recent months and started profiling it a bit. I didn't
see any reason for a regression but did notice that
RewriteRowBlockPointers ends up being memory-bandwidth-bound, so took a
few minutes to optimize it.

The old code did one pass through the data for each column. In the case
that the rowblock was larger than CPU cache, this meant streaming a lot
of data from L2, L3, or even memory, which bottlenecked it. The new code
is branchier but does only a single pass through the data.

This reduced the client side CPU usage of the query in tpch_real_world
from about 70ms/iteration to about 40ms/iteration.

Change-Id: Ie428e595e9f564762bbdbd09dc6a5f312abe9aec
---
M src/kudu/common/wire_protocol.cc
1 file changed, 67 insertions(+), 46 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie428e595e9f564762bbdbd09dc6a5f312abe9aec
Gerrit-Change-Number: 8554
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP [client] expose non-voter replicas for kudu CLI tool

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8586 )

Change subject: WIP [client] expose non-voter replicas for kudu CLI tool
..


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.h@372
PS1, Line 372:
> perhaps make this an enum? SHOW_NON_VOTERS / HIDE_NON_VOTERS
Done


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/client/meta_cache.cc@612
PS1, Line 612: LookupRpc::LookupRpc(scoped_refptr meta_cache,
> warning: pass by value and use std::move [modernize-pass-by-value]
Done


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck.h@161
PS1, Line 161:   void 
set_replicas(std::vector replicas) {
> warning: non-const reference parameter 'replicas', make it const or use a p
Done


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/ksck_remote.cc@318
PS1, Line 318: show_non_voters_
> might be a little friendlier to make this a setter method
The usage pattern for this and rest of the fields is like in a 'private' 
struct.  I was thinking of using getter/setter here, but it would look just for 
one field.


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

http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/tool_action_table.cc@30
PS1, Line 30: #include "kudu/client/client_builder-internal.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/tool_action_table.cc@64
PS1, Line 64: show_non_voters_
> it would be less hacky to use a setter on Data for this
So far all fields of KuduClientBuilder::Data are public, so it's more a struct, 
actually.  I followed the already existing pattern here.  I don't think it's 
worth changing it for just this particular field.


http://gerrit.cloudera.org:8080/#/c/8586/1/src/kudu/tools/tool_action_table.cc@87
PS1, Line 87: is_voter_
> mind putting a getter on this?
Here is the same story with KuduReplica::Data: it's more like a struct with 
constant public fields.  I'm not sure it's worth exposing this particular one 
via a getter.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19317fdf5a2d5c8bb5f37b27bb83067a4df4ea20
Gerrit-Change-Number: 8586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 17 Nov 2017 22:01:53 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [client] expose non-voter replicas for kudu CLI tool

[Alexey Serbin (Code Review)]

Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: WIP [client] expose non-voter replicas for kudu CLI tool
..

WIP [client] expose non-voter replicas for kudu CLI tool

Added private interface to expose non-voter replica in the kudu CLI
tools.  Also, use that functionality to list table's replicas.

I also updated the format of output to make it more readable.  An
example of new output for 'kudu table list --list_tablets':

loadgen_auto_1e5f57a73dd34fa7bde7465219bd1866
  T 69e22ffecb5b459db8a7fbf2f751a76f
L 5acf108a9d364178b1aa672e52214ab8 127.0.0.1:9872
N f2e726948d3143e184097cd953dd2230 127.0.0.1:9878
N 3ae7fdf107cf424dbf9c5e33c33eafd0 127.0.0.1:9876

  T 662bae40e4ec46588e90670bec16b2b5
L 5acf108a9d364178b1aa672e52214ab8 127.0.0.1:9872

  T 5ba95da9b2f9455db9d690bdd35ae5cb
L df8296cf97df42eaacd78e794c028c79 127.0.0.1:9874

  T 528ae88cce1e4415a0d075dead5c983c
L 20e66417713446039c22987fec13b800 127.0.0.1:9870

  T 157ea108357e469e8fee001aec521d7e
L 5acf108a9d364178b1aa672e52214ab8 127.0.0.1:9872

  T 9c6a4d493fff4bda9bfe8f158cb08e69
L 5acf108a9d364178b1aa672e52214ab8 127.0.0.1:9872

  T 36d03adab6ad494ea4e769ef92ef5577
L df8296cf97df42eaacd78e794c028c79 127.0.0.1:9874

  T 7ce6d42984734614bd627d872e99c833
L 20e66417713446039c22987fec13b800 127.0.0.1:9870

WIP: probably, need to update ksck output to explicitly show non-voter
 replicas in a separate column

Change-Id: I19317fdf5a2d5c8bb5f37b27bb83067a4df4ea20
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.cc
M src/kudu/client/client_builder-internal.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/replica-internal.cc
M src/kudu/client/replica-internal.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
15 files changed, 137 insertions(+), 78 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19317fdf5a2d5c8bb5f37b27bb83067a4df4ea20
Gerrit-Change-Number: 8586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] WIP [catalog manager] introduce replica type matching policy

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8161 )

Change subject: WIP [catalog manager] introduce replica type matching policy
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8161/4/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8161/4/src/kudu/integration-tests/raft_consensus-itest.cc@119
PS4, Line 119: using kudu::master::ANY_REPLICA;
> warning: using decl 'ANY_REPLICA' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/8161/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8161/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@73
PS4, Line 73: using kudu::master::VOTER_REPLICA;
> warning: using decl 'VOTER_REPLICA' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
Gerrit-Change-Number: 8161
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 22:01:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1078. Fix 'ops in future' error under load

[Todd Lipcon (Code Review)]

Todd Lipcon has removed a vote on this change.

Change subject: KUDU-1078. Fix 'ops in future' error under load
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie062adcc7fcaa48f09bbe382267d8f755353a443
Gerrit-Change-Number: 8563
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1078. Fix 'ops in future' error under load

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8563 )

Change subject: KUDU-1078. Fix 'ops in future' error under load
..


Patch Set 3: Verified+1

Unrelated test flakes


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie062adcc7fcaa48f09bbe382267d8f755353a443
Gerrit-Change-Number: 8563
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 21:49:37 +
Gerrit-HasComments: No

[kudu-CR] [catalog manager] introduce replica type matching policy

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [catalog manager] introduce replica type matching policy
..

[catalog manager] introduce replica type matching policy

Introduced replica type matching policy for GetTabletLocations RPC and
various wrapper methods used in master, catalog manager, and tests.
The policy is used to specify what type of tablet replicas to locate
while populating the result of the GetTabletLocations RPC.

When not specified otherwise or when using earlier versions of the
GetTable[t]LocationsRequestPB protobuf messages, VOTER_REPLICA policy
is used.  To locate all replicas, specify ANY_REPLICA policy in the
match_policy field of the GetTable[t]LocationsRequestPB.  The latter
is done now unconditionally to enable already existing non-voter
replica-related tests pass.  This will change by a follow-up commit:
the 'regular' client operations should work with voter replicas only,
so the use cases for getting all tablet replicas will be just
the following:

  * the kudu CLI tool fetching that information to report on replica
membership status for listings in 'table list --tablets' and
'ksck' sub-commands.

  * various tests exercising the functionality related to
non-voter tablet replicas

Added an integration test to verify the functionality of the replica
matching policy for GetTableLocations and GetTabletLocations RPC.

Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
---
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
18 files changed, 231 insertions(+), 59 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
Gerrit-Change-Number: 8161
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tablet: make various update paths atomic

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8441 )

Change subject: tablet: make various update paths atomic
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8441/3/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

http://gerrit.cloudera.org:8080/#/c/8441/3/src/kudu/tablet/diskrowset.cc@575
PS3, Line 575:   vector removed_blocks;
> Hrm I've been waffling back and forth between whether it'd be sufficient to
My attempt to justify rollback here:

It seems like the only methods that can update the RowSetMetadata's internal 
state is CommitUpdate(), which currently only gets called while under the 
DeltaTracker's compact_flush_lock (which is maybe an interesting in itself). 
That means that, so long as we're protected by the lock, nothing else will have 
changed the in-memory state, and we should be able to roll back to its 
pre-update state.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0241cd4206ce77ef1fb334458b091bc2092f4141
Gerrit-Change-Number: 8441
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 18 Nov 2017 01:17:40 +
Gerrit-HasComments: Yes

[kudu-CR] schema: micro-optimize num columns()

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8587 )

Change subject: schema: micro-optimize num_columns()
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52d63e17d77c0b35685aa3cfba51e6905308592d
Gerrit-Change-Number: 8587
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Nov 2017 22:29:34 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Anonymous Coward (Code Review)]

saketa.chalamch...@gmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6010 )

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..


Patch Set 3:

java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala

- Reflowed all lines to be < 100 characters
- Removed comments
- Introduced constants for property names
- "Line 125:
how does this 'AnyRef' thing work? are you sure this works with all types? Can 
you try something like an int8 column and passing an integer?"

AnyRef doesn't work with BinaryType and returns and incorrectly converts 
integer to int8 in the scenario mentioned above.
Therefore, removed "AnyRef" and cast DefaultValue to it's right type using 
Column Type as reference. Appropriate errors are thrown in case of type 
mismatch.

- Encoding and Compression Algorithm values are converted to Upper case to 
accomodate for both upper and lower characters in input

Removed
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextWithMetadataTest.scala

Added test cases under 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala

- Reflowed all lines to be < 100 characters
- Added test cases for valid and invalid metadata columns
- Followed variable names used in other test cases
- "Is there a nicer way to specify the metadata than using fromJson? I'm 
surprised you can't pass it as a scala map."

There is. Kind of. The MetaDataBuilder can build metadata with the following 
put methods
putBoolean
putBooleanArray
putDouble
putDoubleArray
putLong
putLongArray
putMetadata
putMetadataArray
putString
putStringArray


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 3
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: saketa.chalamch...@gmail.com
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:11:22 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6010 )

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..


Patch Set 3:

> java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
 >
 > - Reflowed all lines to be < 100 characters
 > - Removed comments
 > - Introduced constants for property names
 > - "Line 125:
 > how does this 'AnyRef' thing work? are you sure this works with all
 > types? Can you try something like an int8 column and passing an
 > integer?"
 >
 > AnyRef doesn't work with BinaryType and returns and incorrectly
 > converts integer to int8 in the scenario mentioned above.
 > Therefore, removed "AnyRef" and cast DefaultValue to it's right
 > type using Column Type as reference. Appropriate errors are thrown
 > in case of type mismatch.
 >
 > - Encoding and Compression Algorithm values are converted to Upper
 > case to accomodate for both upper and lower characters in input
 >
 > Removed
 > java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextWithMetadataTest.scala
 >
 > Added test cases under 
 > java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
 >
 > - Reflowed all lines to be < 100 characters
 > - Added test cases for valid and invalid metadata columns
 > - Followed variable names used in other test cases
 > - "Is there a nicer way to specify the metadata than using
 > fromJson? I'm surprised you can't pass it as a scala map."
 >
 > There is. Kind of. The MetaDataBuilder can build metadata with the
 > following put methods
 > putBoolean
 > putBooleanArray
 > putDouble
 > putDoubleArray
 > putLong
 > putLongArray
 > putMetadata
 > putMetadataArray
 > putString
 > putStringArray

Does it make sense to start a new code review? since the base is out of date 
and is hard to tell what are your changes on the diff.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 3
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: saketa.chalamch...@gmail.com
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:18:31 +
Gerrit-HasComments: No

[kudu-CR] catalog manager tsk-itest: ensure that test eventually makes progress

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8567 )

Change subject: catalog_manager_tsk-itest: ensure that test eventually makes 
progress
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
Gerrit-Change-Number: 8567
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:40:17 +
Gerrit-HasComments: No

[kudu-CR] [spark] Remove AsyncClient in KuduContext

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8551 )

Change subject: [spark] Remove AsyncClient in KuduContext
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8551/1//COMMIT_MSG@7
PS1, Line 7: [spark] Remove AsyncClient in KuduContext
> Can we not just update both the sync and async clients? And propagate the m
Now we only update sync client for timestamp propagation and I don't see in 
this way it would affect the correctness? I thought the concern is to avoid 
misuse in future? (Maybe I have missed something)

If we are worrying about compatibility,  I remember Dan suggested another way 
is to redefine AsyncClient so that they are sharing the same client instance 
underneath.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8745d8503a35e17a632d0a0cde5738915f00fb
Gerrit-Change-Number: 8551
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 17 Nov 2017 22:17:23 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Anonymous Coward (Code Review)]

saketa.chalamch...@gmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6010 )

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..


Patch Set 3:

> (1 comment)

Changed the commit message to
Bug #

Description

Change-ID#


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 3
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: saketa.chalamch...@gmail.com
Gerrit-Comment-Date: Fri, 17 Nov 2017 22:53:30 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Anonymous Coward (Code Review)]

Hello Chris George, Dan Burkert, Kudu Jenkins, Hao Hao, Todd Lipcon,

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

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

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

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..

KUDU-1737 : Submit column characteristics via KuduContext

Submit defaultValue, desiredBlockSize, encoding and
compressionAlgorithm via metadata in schema when using KuduContext's
createTable.

Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
2 files changed, 443 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/6010/3
--
To view, visit http://gerrit.cloudera.org:8080/6010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 3
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1737 : Submit column characteristics via KuduContext

[Anonymous Coward (Code Review)]

saketa.chalamch...@gmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6010 )

Change subject: KUDU-1737 : Submit column characteristics via KuduContext
..


Patch Set 3:

> > java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
 > >
 > > - Reflowed all lines to be < 100 characters
 > > - Removed comments
 > > - Introduced constants for property names
 > > - "Line 125:
 > > how does this 'AnyRef' thing work? are you sure this works with
 > all
 > > types? Can you try something like an int8 column and passing an
 > > integer?"
 > >
 > > AnyRef doesn't work with BinaryType and returns and incorrectly
 > > converts integer to int8 in the scenario mentioned above.
 > > Therefore, removed "AnyRef" and cast DefaultValue to it's right
 > > type using Column Type as reference. Appropriate errors are
 > thrown
 > > in case of type mismatch.
 > >
 > > - Encoding and Compression Algorithm values are converted to
 > Upper
 > > case to accomodate for both upper and lower characters in input
 > >
 > > Removed
 > > java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextWithMetadataTest.scala
 > >
 > > Added test cases under 
 > > java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
 > >
 > > - Reflowed all lines to be < 100 characters
 > > - Added test cases for valid and invalid metadata columns
 > > - Followed variable names used in other test cases
 > > - "Is there a nicer way to specify the metadata than using
 > > fromJson? I'm surprised you can't pass it as a scala map."
 > >
 > > There is. Kind of. The MetaDataBuilder can build metadata with
 > the
 > > following put methods
 > > putBoolean
 > > putBooleanArray
 > > putDouble
 > > putDoubleArray
 > > putLong
 > > putLongArray
 > > putMetadata
 > > putMetadataArray
 > > putString
 > > putStringArray
 >
 > Does it make sense to start a new code review? since the base is
 > out of date and is hard to tell what are your changes on the diff.

I'll make a new change request


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbf60d61ac3f7e14b91fd2f83137c2dc8f1ecd5
Gerrit-Change-Number: 6010
Gerrit-PatchSet: 3
Gerrit-Owner: saketa.chalamch...@gmail.com
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: saketa.chalamch...@gmail.com
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:21:54 +
Gerrit-HasComments: No

[kudu-CR] catalog manager tsk-itest: ensure that test eventually makes progress

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8567 )

Change subject: catalog_manager_tsk-itest: ensure that test eventually makes 
progress
..

catalog_manager_tsk-itest: ensure that test eventually makes progress

This test previously tried to introduce a lot of master leader elections
by setting a very low heartbeat and failure interval. This worked, but
sometimes worked so well that the test never made progress and couldn't
obtain a stable leader long enough to create a table.

This patch changes the test to instead use a separate thread which
triggers elections manually on all the leaders. The elections start off
very frequent and then back off as the test progresses to ensure that by
the end, the leaders do actually make progress.

I verified that this still covers the case of a failed write when
writing TSKs by changing the RETURN_NOT_OK to a CHECK_OK when storing
the TSK. With the CHECK_OK, the test failed nearly immediately.

Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
Reviewed-on: http://gerrit.cloudera.org:8080/8567
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/integration-tests/catalog_manager_tsk-itest.cc
1 file changed, 46 insertions(+), 20 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3ecda0c269225e7674bc384fee652576b110ae7b
Gerrit-Change-Number: 8567
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Bug: KUDU-1737

[Anonymous Coward (Code Review)]

saketa.chalamch...@gmail.com has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8591


Change subject: Bug: KUDU-1737
..

Bug: KUDU-1737

Submit defaultValue, desiredBlockSize, encoding and
compressionAlgorithm via metadata in schema when using KuduContext's
createTable.

Change-Id: Idfca6018e68cf654141e381dde167b0e99b7b07a
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
2 files changed, 316 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idfca6018e68cf654141e381dde167b0e99b7b07a
Gerrit-Change-Number: 8591
Gerrit-PatchSet: 1
Gerrit-Owner: saketa.chalamch...@gmail.com

[kudu-CR] KUDU-2215. kernel stack watchdog: avoid blocking thread exit

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8536 )

Change subject: KUDU-2215. kernel_stack_watchdog: avoid blocking thread exit
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6a349666e8484c00b2f43c5918205ec1a4c09ab
Gerrit-Change-Number: 8536
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 18 Nov 2017 04:27:00 +
Gerrit-HasComments: No

[kudu-CR] [catalog manager] introduce replica type matching policy

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [catalog manager] introduce replica type matching policy
..

[catalog manager] introduce replica type matching policy

Introduced replica type matching policy for GetTabletLocations RPC and
various wrapper methods used in master, catalog manager, and tests.
The policy is used to specify what type of tablet replicas to locate
while populating the result of the GetTabletLocations RPC.

When not specified otherwise or when using earlier versions of the
GetTable[t]LocationsRequestPB protobuf messages, VOTER_REPLICA policy
is used.  To locate all replicas, specify ANY_REPLICA policy in the
match_policy field of the GetTable[t]LocationsRequestPB.  The latter
is done now unconditionally to enable already existing non-voter
replica-related tests pass.  This will change by a follow-up commit:
the 'regular' client operations should work with voter replicas only,
so the use cases for getting all tablet replicas will be just
the following:

  * the kudu CLI tool fetching that information to report on replica
membership status for listings in 'table list --tablets' and
'ksck' sub-commands.

  * various tests exercising the functionality related to
non-voter tablet replicas

Added an integration test to verify the functionality of the replica
matching policy for GetTableLocations and GetTabletLocations RPC.

Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
---
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
18 files changed, 232 insertions(+), 59 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
Gerrit-Change-Number: 8161
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon