[kudu-CR] KUDU-1982 Java client calls NetworkInterface.getByInetAddress too often NetworkInterface.getByInetAddress (which is used by NetUtil.isLocalAddress) is very slow on Windows, so cache added.

2017-04-26 Thread Pavel Martynov (Code Review) 
Pavel Martynov has posted comments on this change.

Change subject: KUDU-1982 Java client calls NetworkInterface.getByInetAddress 
too often NetworkInterface.getByInetAddress (which is used by 
NetUtil.isLocalAddress) is very slow on Windows, so cache added.
..


Patch Set 1:

(1 comment)

Reply on TestServerInfo.java comments

http://gerrit.cloudera.org:8080/#/c/6735/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java:

Line 12:   @Test(timeout = 500)
> was it previously so slow that this timeout would fire?
Yes, without my patch this test runs about 5 seconds on my desktop. Also, I 
checked NetworkInterface.getNetworkInterfaces() on my Windows desktop and 
laptop, it returns 35 and 41 respectively (most of them - some virtual 
interfaces, not manageable by a user).

Slow DNS respond is not a problem, because it simply not performed: "If a 
literal IP address is supplied, only the validity of the address format is 
checked." (see 
http://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getByName%28java.lang.String%29).

Yes, I already verified on my initial insert-loadgen sample program and insert 
pace increased by 15-20 times.

Well, I am also not a big fan of such tests, so I understand you correctly that 
is better to remove it completely?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8fc9d40ebffa17ef43973dc23237dd31dd309c06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Pavel Martynov 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Pavel Martynov 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] external mini cluster: spawn perf record for each daemon during Start()

2017-04-26 Thread Adar Dembo (Code Review) 
Hello David Ribeiro Alves, Todd Lipcon,

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

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

to review the following change.

Change subject: external mini cluster: spawn perf record for each daemon during 
Start()
..

external mini cluster: spawn perf record for each daemon during Start()

I want this for the dense node test, but it's not really possible to do from
outside ExternalMiniCluster without missing out on time spent in Start(),
which I was interested in measuring. So here's a generic approach that can
be used by any itest.

I wasn't sure whether this should be configured via EMC option or gflag. I
ended up with the latter because it's not really something a test needs
programmatic access to; it's just something the dev running the test might
want to enable manually.

Change-Id: I92079f616648788b461d550057b8e23bc9174b71
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
2 files changed, 41 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I92079f616648788b461d550057b8e23bc9174b71
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] subprocess: remove progname argument

2017-04-26 Thread Adar Dembo (Code Review) 
Hello David Ribeiro Alves, Todd Lipcon,

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

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

to review the following change.

Change subject: subprocess: remove progname argument
..

subprocess: remove progname argument

When constructing a subprocess, 'argv[0]' always duplicates 'program', and
is typically BaseName()'d as well. Let's just codify that in the subprocess
code so that callers need not worry about it.

Change-Id: I42d3cb551c350d8f10308035084a8807a1ae240b
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/pstack_watcher.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 33 insertions(+), 46 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I42d3cb551c350d8f10308035084a8807a1ae240b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] subprocess: add ScopedSubprocess

2017-04-26 Thread Adar Dembo (Code Review) 
Hello David Ribeiro Alves, Todd Lipcon,

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

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

to review the following change.

Change subject: subprocess: add ScopedSubprocess
..

subprocess: add ScopedSubprocess

This patch introduces an RAII wrapper around a Subprocess, best suited for
processes that should run alongside a unit test and should be killed
indiscriminately when the test ends.

I intend to use this in the dense node test for "perf", and I changed the
existing perf calls in full_stack-insert-scan-test to use it. While there I
fixed the handling of "--call-graph"; passing it without "fp" is a syntax
error on both el6.6 and Ubuntu 16.04.

Change-Id: Iaf31bd4ea6de2917521a9852714fb33cfbec1f61
---
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.h
3 files changed, 99 insertions(+), 30 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf31bd4ea6de2917521a9852714fb33cfbec1f61
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-579 [java client] Scanner fault tolerance

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-579 [java_client] Scanner fault tolerance
..


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6566/8/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

PS8, Line 731: final
nit: doesn't need to be final.


http://gerrit.cloudera.org:8080/#/c/6566/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:

PS8, Line 124: .
nit, remove


http://gerrit.cloudera.org:8080/#/c/6566/8/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java:

Line 515: error.getCode() == 
Tserver.TabletServerErrorPB.Code.TABLET_NOT_RUNNING) {
master.proto says:
> The request or response involved a tablet which is not yet running.

Shouldn't we be calling handleRetryableError instead? Basically we just need to 
check back later.


http://gerrit.cloudera.org:8080/#/c/6566/8/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
File 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java:

Line 132: 
Can you add tests that do the same as below but that aren't fault tolerant?


Line 159:   private void verifyFaultTolerantScanner(boolean shouldRestart) 
throws Exception {
Can you also add tests that kill/restart servers after we're done scanning the 
first tablet? Below only makes sure that we can continue in the middle of a 
tablet.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Improve unsupported application feature flags error status

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: Improve unsupported application feature flags error status
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie59fba2b5ff57a22d16c7b7eca55ab4581e9b64c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Improve unsupported application feature flags error status

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: Improve unsupported application feature flags error status
..


Improve unsupported application feature flags error status

It can be helpful to have the exact set of unsupported flags when
debugging compatibility issues.

Change-Id: Ie59fba2b5ff57a22d16c7b7eca55ab4581e9b64c
Reviewed-on: http://gerrit.cloudera.org:8080/6722
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/rpc/service_pool.cc
1 file changed, 6 insertions(+), 2 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie59fba2b5ff57a22d16c7b7eca55ab4581e9b64c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-861 Support changing column defaults and storage attributes

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-861 Support changing column defaults and storage attributes
..


Patch Set 3: Code-Review+1

Carrying my +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48dcfd1ced488943c3da1eb0a26f62780ac9214f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [docs] Add troubleshooting for KuduStorageHandler

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Hello Dan Burkert,

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

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

to review the following change.

Change subject: [docs] Add troubleshooting for KuduStorageHandler
..

[docs] Add troubleshooting for KuduStorageHandler

Change-Id: I80e028a6f827269d97f26ec7a2cf4b8c22d2a838
---
M docs/troubleshooting.adoc
1 file changed, 10 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I80e028a6f827269d97f26ec7a2cf4b8c22d2a838
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Dan Burkert

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

2017-04-26 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change.

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..


Patch Set 7:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/6514/5//COMMIT_MSG
Commit Message:

PS5, Line 9: 
   : This rejects unauthenticated connections from publicly routable 
IPs,
   : even if authentication and encryption are not configured. An unsafe
   : flag 'allow_unauthenticated_public_connections' is provided to 
enable
   : unauthentic
> nit: could you keep the lines under 72 chars length?
Done


http://gerrit.cloudera.org:8080/#/c/6514/1/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:

Line 68: DEFINE_bool(allow_unauthenticated_public_connections, false,
> I re-ran the checks with testing public connectivity to some of their used 
Thanks Harsh! I am bringing this discussion to dev mailing list.


http://gerrit.cloudera.org:8080/#/c/6514/5/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:

PS5, Line 149: RETURN_NOT_OK(RejectUntrustedPublicConnection(addr));
 :   }
> nit: why not just
Done


PS5, Line 679:   negotiated_mech_ == SaslMechanism::PLAIN) {
 : Sockaddr addr;
> RETURN_NOT_OK(RejectUntrustedPublicConnection(addr));
Done


PS5, Line 858: 
> nit: Status::OK() is set by the default constructor, so no assignment is ne
Done


PS5, Line 861: rivateAddress()
> are prohibited
Done


http://gerrit.cloudera.org:8080/#/c/6514/5/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:

PS5, Line 114: Sockaddr::IsPrivateAddress
> What about link-local addresses like 169.254.0.0/16 except for first and la
Yeah, thanks for point it out. I think we should consider it as private 
address. This is under discussion in the dev mailing list.


PS5, Line 115:   uint32 first_byte, sec_byte;
 :   first_byte = 
NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 24;
 :   sec_byte = 
(NetworkByteOrder::FromHost32(addr_.sin_addr.s_addr) >> 16) & 0xff;
> nit: why not to initialize the variables at the point of definition, like 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

2017-04-26 Thread Hao Hao (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..

KUDU-1875: Refuse unauthenticated connections from publicly routable
IP addrs

This rejects unauthenticated connections from publicly routable IPs,
even if authentication and encryption are not configured. An unsafe
flag 'allow_unauthenticated_public_connections' is provided to enable
unauthenticated connections from publicly routable IPs. Even though it
is highly recommended to disable it.

Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
---
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket.h
8 files changed, 142 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-861 Support changing column defaults and storage attributes

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-861 Support changing column defaults and storage attributes
..


Patch Set 2: Code-Review+1

Java side looks good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48dcfd1ced488943c3da1eb0a26f62780ac9214f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1982 Java client calls NetworkInterface.getByInetAddress too often NetworkInterface.getByInetAddress (which is used by NetUtil.isLocalAddress) is very slow on Windows, so cache added.

2017-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1982 Java client calls NetworkInterface.getByInetAddress 
too often NetworkInterface.getByInetAddress (which is used by 
NetUtil.isLocalAddress) is very slow on Windows, so cache added.
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6735/1/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:

PS1, Line 51: if (isLocalAddressCache.containsKey(resolvedAddr)) {
:   this.local = isLocalAddressCache.get(resolvedAddr);
better to avoid the double lookup here, and instead just do a 'get' and check 
the result against null (it's a boxed boolean so would return null if missing).

Boolean isLocal = ...get(resolvedAddr);
if (isLocal == null) {
  isLocal = ...isLocal();
  ..put(resolvedAddr, isLocal);
}
this.local = isLocal

something like the above


http://gerrit.cloudera.org:8080/#/c/6735/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java:

Line 1: package org.apache.kudu.client;
add license header


Line 12:   @Test(timeout = 500)
was it previously so slow that this timeout would fire?

I'm nervous that this test will be flaky -- eg if DNS is slow to respond on a 
machine, would it potentially take >500ms for a single lookup?

Perhaps better to not bother with a test for this fix, since the codepath is 
well covered by existing tests, and it should really be tested by a perf 
regression test. Unfortunately we don't have any infra for setting up regular 
Windows perf tests, but would be great if you can verify that this fixed the 
issue on your box.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8fc9d40ebffa17ef43973dc23237dd31dd309c06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Pavel Martynov 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [KUDU-754] add an environment variable for kudu client debugging to stderr

2017-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: [KUDU-754] add an environment variable for kudu client 
debugging to stderr
..


Patch Set 1:

(12 comments)

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

Line 9: read environment variable "KUDU_CLIENT_VERBOSE", calls 
SetVerboseLogLevel to set the specific verbose level. The 
InitiGoogleLoggingSafeBasic() already sets the debugging to stderr.
nit: can you re-wrap and format this into full sentences with appropriate 
capitalization, etc? https://chris.beams.io/posts/git-commit/ has a good guide


http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 5049: 
nit: whitespace here and below


Line 5050: // Test that, if a different verbose level through environment 
varialble is set, it reflects to the FLAGS_v
nit: please wrap (here and a few lines down as well)


Line 5053:   char sample_verboseLevel[] = "5";  // select a specific verbose 
level different from 0
no need for a separate variable here. I think it's clear what you're doing so 
the comment can also be removed.


Line 5059:   ASSERT_EQ(std::atoi(sample_verboseLevel), FLAGS_v);
I think it's fine to just use '5' here literal instead of std::atoi() it above, 
since the test is so short it doesn't really aid clarity


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

PS1, Line 148: kLevel
this isn't a constant, so it should just be 'int level'


PS1, Line 149: char const* temp 
const char*?


Line 151:   {
style: { goes on the same line as 'if'. Same below.


Line 152: kLevel = std::atoi(temp);
use safe_strto32() from numbers.h instead, so you can check validity. Maybe 
LOG(WARNING) if the env var is invalid?


PS1, Line 153: kLevel <= 6
why <= 6? Is there some limit on the vlog level as far as glog is concerned?


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

PS1, Line 125: /// Set the logging verbose level through environment variable
 : void KUDU_EXPORT SetClientVerboseLevelFromEnvironmentVariable();
given this is called by the client during initialization, I don't think it's 
necessary to export it as a public API. If you put it here just for testing, 
you could move it to client-internal.h (which the tests are allowed to include 
but don't get shipped)


Line 129: static const char* kClientVerboseEnvironmentVariable = 
"KUDU_CLIENT_VERBOSE";
This should probably be declared extern in the header, and defined in the .cc 
file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab5c7c24395c25184489200283dd38da024c07bb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: William Li 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [KUDU-754] add an environment variable for kudu client debugging to stderr

2017-04-26 Thread William Li (Code Review) 
William Li has uploaded a new change for review.

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

Change subject: [KUDU-754] add an environment variable for kudu client 
debugging to stderr
..

[KUDU-754] add an environment variable for kudu client debugging to stderr

read environment variable "KUDU_CLIENT_VERBOSE", calls SetVerboseLogLevel to 
set the specific verbose level. The InitiGoogleLoggingSafeBasic() already sets 
the debugging to stderr.

Change-Id: Iab5c7c24395c25184489200283dd38da024c07bb
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
3 files changed, 35 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab5c7c24395c25184489200283dd38da024c07bb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: William Li

[kudu-CR] Make NO FATALS work standalone

2017-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: Make NO_FATALS work standalone
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6730/1/src/kudu/util/test_macros.h
File src/kudu/util/test_macros.h:

Line 23: // ASSERT_NO_FATAL_FAILURE is just too long to type.
> This needs to be updated; NO_FATALS is no longer just shorthand, it now is 
would it be clearer to leave this as is, and add a new #define like 
'NO_PENDING_FATALS' or something which does the latter half of this? (26-28) 
and use that in all cases where we have a no-argument form of NO_FATALS()?


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

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

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has submitted this change and it was merged.

Change subject: Simplify MemTracker and move process throttling elsewhere
..


Simplify MemTracker and move process throttling elsewhere

This takes a first step towards simplifying MemTracker:

- Remove the "GC function" callbacks (we never used this)

- Remove the 'ExpandLimits' code which was unimplemented.

- Remove the logging functionality, which we've never used
  as far as I can remember.

- Remove soft limiting. We only used this on the root tracker, so
  I just moved it to a new process_memory.{h,cc}

- Remove 'consumption_func' and un-tie the root tracker from
  the global process memory usage. Now the root tracker is a simple
  sum of its descendents.

For a stress/benchmark I ran a 500GB YCSB with a memory limit set to
10GB. Results showed no major difference with this patch (throughput was
a few percent faster but within the realm of noise). Details at [1]

[1] 
https://docs.google.com/document/d/1dOe5-L5BWUhF-uV4-AE5hduvfUWctXizlaoSLlp38yM/edit?usp=sharing

Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Reviewed-on: http://gerrit.cloudera.org:8080/6620
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
14 files changed, 373 insertions(+), 613 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tpch: allow hash partitioning

2017-04-26 Thread Todd Lipcon (Code Review) 
Todd Lipcon has submitted this change and it was merged.

Change subject: tpch: allow hash partitioning
..


tpch: allow hash partitioning

This enables a hash-partitioning mode for tpch_real_world. In this mode,
we still create one tablet per thread, but we use hash-partitioning so
that we don't get the perfect one-to-one insert pattern that we get with
range partitioning.

This is a bit more realistic for many batch insert scenarios, in which
many writers write locally-sorted (but overlapping) data ranges into a
single hash-partitioned table.

Change-Id: Icbbb643447dff58b32e67764f6fb632441a4f6e4
Reviewed-on: http://gerrit.cloudera.org:8080/6709
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.h
M src/kudu/benchmarks/tpch/tpch1.cc
M src/kudu/benchmarks/tpch/tpch_real_world.cc
5 files changed, 59 insertions(+), 15 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Icbbb643447dff58b32e67764f6fb632441a4f6e4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) FAQ refresh

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has posted comments on this change.

Change subject: FAQ refresh
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6697/1/faq.md
File faq.md:

Line 194
> Why was this removed? Don't we still have recommendations on maximum cell s
I moved the limitations stuff to the new limitations doc.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd8a79e3067bedbef71b0dcf9921be673e9dfaa0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) FAQ refresh

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: FAQ refresh
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6697/1/faq.md
File faq.md:

Line 194
Why was this removed? Don't we still have recommendations on maximum cell size?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd8a79e3067bedbef71b0dcf9921be673e9dfaa0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: WIP: KUDU-463. Add checksumming to cfile
..


Patch Set 10:

(21 comments)

I didn't review the checksumming stuff yet, just looked at the rest and the 
plumbing.

Since the ReadV() stuff turned out to be pretty substantial, could you move it 
into its own commit? Would be nice to finish reviewing it separately.

http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/cfile/cfile_reader.h
File src/kudu/cfile/cfile_reader.h:

Line 210:   bool has_checksums_;
This will be padded to 8 bytes, which is actually quite a bit given how many 
CFileReaders we instantiate. Perhaps we can calculate this on the fly instead, 
using a mask on footer_'s features?


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

PS10, Line 262: gscoped_ptr
Nit: use unique_ptr.

Actually, these scratch spaces are tiny; can you just allocate them on the 
stack?


Line 267:   ReadResult {
Surprised that this style of struct initialization works; isn't it C99 but not 
C++11 compatible?


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 38: class Env;
No need for this anymore.

Though if you decide to pass the vector by pointer as I suggested in env.h, you 
can omit the env.h include, forward declare ReadResult, and continue to forward 
declare Env.


PS10, Line 162: result
You mean results?


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/fs/fs-test-util.h
File src/kudu/fs/fs-test-util.h:

PS10, Line 72: vector
This is a header so we retain namespace qualifications (i.e. this should be 
std::vector).


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/util/crc.cc
File src/kudu/util/crc.cc:

PS10, Line 50:  
Nit: extra space here.


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/util/crc.h
File src/kudu/util/crc.h:

PS10, Line 38: crc32
Maybe previous_crc32 to be more clear?


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 371: struct ReadResult {
Should provide a short high-level struct document too.


PS10, Line 403: results
I'm finding this to be somewhat confusing.

Part of the issue is with the name ReadResult: the majority of stuff in it 
aren't "results"; they're actually input to the function. Maybe ReadEntry or 
ReadVEntry would be better?

Part of the issue is that the Slice* means an extra level of indirection in the 
setup that callers have to do. Perhaps they can be regular Slices, that are 
default-initialized by the caller and set by the callee? That means 'results' 
should be passed by pointer, but I think that's a clearer model anyway, since 
we pass by pointer things that the function is expected to modify.

Also, could you reorganize ReadResult so that the pure IN parameters precede 
IN/OUT and OUT? Meaning, the order should probably be length, scratch, then 
result.


Line 536:   // TODO (GH): Document
Yep. :)


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

PS10, Line 175: const struct iovec *
Should be const struct iovec*


Line 180:   return read_bytes;
Nit: if you're going to elide the braces, merge this line into the conditional 
line. This makes it harder to accidentally mess up the conditional if another 
line is added.


Line 182:   break;
Don't we need to update total_read_bytes with our partial read results before 
breaking?


Line 334:   virtual Status ReadV(uint64_t offset, vector results) 
const OVERRIDE {
Could you add TRACE_EVENT1() calls to ReadV() and Read()? Not sure why they 
were missing in the first place.


PS10, Line 340: ReadResult
Can be cref?


Line 341:   return iovec {
Again, surprised this kind of initialization works in C++11.


PS10, Line 347: results.size()
arraylength(iov) is more idiomatic.


Line 353: // Adjust slice sizes based on bytes read for short reads
As we discussed, let's make short reads an error, for Read() too. Do it as a 
separate patch and layer this one on top of it.


PS10, Line 355: ReadResult
Nit: auto is fine for short-scoped types like this one.


http://gerrit.cloudera.org:8080/#/c/6630/10/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:

Line 66: Status ReadVFully(RandomAccessFile* file, uint64_t offset, 
std::vector results);
If you end up solving the partial read stuff directly in RandomAccessFile, this 
won't be necessary.

But if it is, please also doc it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke

[kudu-CR] [rpc] reopen outbound connections mode for reactor

2017-04-26 Thread Alexey Serbin (Code Review) 
Alexey Serbin has submitted this change and it was merged.

Change subject: [rpc] reopen_outbound_connections mode for reactor
..


[rpc] reopen_outbound_connections mode for reactor

Introduced a test mode for the RPC ReactorThread: reopen already
existing idle outbound connection upon making another remote call
to the same server.  This is a groundwork for follow-up patches which
contain tests requiring connection negotiation to be done upon every
RPC call.

Added a couple of new ReactorThread metrics to count total number of
server- and client-side connections open during ReactorThread lifetime.

Added unit test to cover the newly introduced functionality.  Also, did
a minor clean-up on the ReactorThread code and its inline documentation.

Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Reviewed-on: http://gerrit.cloudera.org:8080/6710
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
5 files changed, 102 insertions(+), 24 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1853 (redux). tablet copy: Don't orphan blocks on failure

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: KUDU-1853 (redux). tablet copy: Don't orphan blocks on failure
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6733/1/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:

Line 187: vector GetAllSortedBlocks(const tablet::TabletSuperBlockPB& 
sb) {
This can use the new test method you added to get all the blocks, right?


PS1, Line 218:   // Verify that the new superblock reflects the changes in 
block IDs.
 :   //
 :   // As long as block IDs are generated with UUIDs or something 
equally
 :   // unique, there's no danger of a block in the new superblock 
somehow
 :   // being assigned the same ID as a block in the existing 
superblock.
This comment is out of date, I think? Also, you aren't relying on the sorted 
nature of GetAllSortedBlocks(); you could use the new test method you added 
that doesn't sort.


PS1, Line 288: FIXME: The data loss check here will rarely (never) trigger
 :   // until we fix KUDU-1980.
Not sure I understand this. AFAICT tablet copy doesn't use the block cache at 
all, so why is KUDU-1980 related?

Also, reformat as TODO(mpercy).


http://gerrit.cloudera.org:8080/#/c/6733/1/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:

Line 30: #include "kudu/gutil/gscoped_ptr.h"
Not used?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic999ccd27859ace0d635255e86729dff8e1d8349
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tablet copy: Ensure no data loss on tablet copy failure

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: tablet copy: Ensure no data loss on tablet copy failure
..


Patch Set 1:

(1 comment)

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

PS1, Line 9: This patch adds an integration test to ensure that we don't see 
data
   : loss on tablet copy failure. Tablet copy failure is triggered a 
couple
   : of different ways at the source server side.
If you unrevert the patch that Todd reverted, does this test fail in the 
desired manner?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie57590e2473e09a6836bb02d5fccb7689614f8e7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tablet copy: Ensure no data loss on tablet copy failure

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: tablet copy: Ensure no data loss on tablet copy failure
..


Patch Set 1:

(7 comments)

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

Line 969:   const char* kTableA = "table_a";
Move this down to L976.


Line 985:   const char* kTableB = "table_b";
Move down.


Line 1000:   // Shut down replica with only [A].
Maybe before doing this, assert that you got the replica distribution that you 
wanted?


Line 1022: // The early timeout flag will also cause the tablet copy to 
fail, but
ASSERT that failure_flag is the other flag.


http://gerrit.cloudera.org:8080/#/c/6732/1/src/kudu/tserver/tablet_copy_service.cc
File src/kudu/tserver/tablet_copy_service.cc:

Line 69: TAG_FLAG(tablet_copy_early_session_timeout_prob, unsafe);
Tag it hidden too. The rest as well (instead of unsafe), if they're supposed to 
be testing-only.


Line 162:   // For testing: Close the session prematurely if unsafe gflag is 
set but
Is there a particular reason why this must happen here (after the session has 
been created) vs. above, where responding with a failure doesn't require 
tearing down a session? Plus responding earlier with RPC_RETURN_NOT_OK means 
app_error will actually make it back to the client.


Line 288: CHECK_OK(DoEndTabletCopySessionUnlocked(session_id, _error));
Should probably be WARN_NOT_OK.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie57590e2473e09a6836bb02d5fccb7689614f8e7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tablet copy: Refactor to avoid separate expiration map

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: tablet copy: Refactor to avoid separate expiration map
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6731/1/src/kudu/tserver/tablet_copy_service.cc
File src/kudu/tserver/tablet_copy_service.cc:

Line 122: const auto& iter = sessions_.find(session_id);
Couldn't you still use FindCopy() as long as the third argument was a 
SessionEntry? I think it's a little more idiomatic than find() + comparison 
against end().


Line 134:   InsertOrDie(_, session_id, { .session = session, 
.expires = GetNewExpireTime() });
I thought this was illegal in C++11:

http://stackoverflow.com/questions/18731707/why-does-c11-not-support-designated-initializer-list-as-c99

If it compiles for you, can you make sure it's not because of a GNU extension?


PS1, Line 261:   for (const auto& entry : sessions_) {
 : session_ids.push_back(entry.first);
 :   }
 :   for (const string& session_id : session_ids) {
Can these loops be merged?


Line 276:   const auto& entry = sessions_.find(session_id);
Here too.


Line 319:   auto iter = sessions_.find(session_id);
Here maybe FindOrNull, so you can get a pointer to the SessionEntry in order to 
modify the expiration time?


http://gerrit.cloudera.org:8080/#/c/6731/1/src/kudu/tserver/tablet_copy_service.h
File src/kudu/tserver/tablet_copy_service.h:

Line 49: struct SessionEntry {
Maybe declare this as a nested and private member of TabletCopyServiceImpl, 
since it's not supposed to be used by anyone except the class itself?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idcf4e57722e4e34e99064713a6e8d246d3ebf920
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] Make NO FATALS work standalone

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: Make NO_FATALS work standalone
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6730/1/src/kudu/util/test_macros.h
File src/kudu/util/test_macros.h:

Line 23: // ASSERT_NO_FATAL_FAILURE is just too long to type.
This needs to be updated; NO_FATALS is no longer just shorthand, it now is more 
featureful (in that it can work on empty statements while 
ASSERT_NO_FATAL_FAILURES can't.

IIRC the original problem was that ASSERT_NO_FATAL_FAILURES doesn't compile 
properly when the statement includes a lambda, right? Ideally that's something 
that should be addressed upstream (or by us, in a patch to gtest). I took a 
quick look but couldn't find any bugs like this in the upstream bug tracker; 
perhaps you can file one and link to it here?

Separately, is there anything we can do to prevent the use of 
NO_FATALS/ASSERT_NO_FATAL_FAILURE on a lambda? The compile failure wasn't 
universal; IIRC it only affected newer gcc. See commit 7034bff for details.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia004214d21a09ca0523ea11d88d2cd4ff783a1ca
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] tpch: allow hash partitioning

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: tpch: allow hash partitioning
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icbbb643447dff58b32e67764f6fb632441a4f6e4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-26 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: Simplify MemTracker and move process throttling elsewhere
..


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:

Line 240:   if (bytes == 0) {
> I don't think Consume() can fail (only TryConsume fails). Consume always su
Ah yes, my bad. Was looking at TryConsume() when I wrote that comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

2017-04-26 Thread Grant Henke (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: WIP: KUDU-463. Add checksumming to cfile
..

WIP: KUDU-463. Add checksumming to cfile

Adds optional checksumming and validation to cfile blocks.

Introduces 2 flags to control cfile checksumming:
- cfile_write_checksums (default false)
- cfile_verify_checksums (default true)

cfile_write_checksums is used in the CFileWriter to enable computing and
appending Crc32 checksums to the end of each cfile block, header and footer

cfile_write_checksums is defaulted to false to ensure upgrades don't
immediately result in performance degredation and incompatible data on
downgrade. It can and likely should be defaulted to true in a later release.

When cfile_write_checksums is set to true, the existing incompatible_features
bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear
error message when older versions of Kudu try to read the file. If checksums
are not written the incompatible_features "checksum" bit is not set.

cfile_verify_checksums is used in the CFileReader to enable validating the
data against the written checksum. cfile_verify_checksums is defaulted to
true since validation only occurs if checksums are written. Any data that
was written before checksumming was an option or when cfile_write_checksums
was false will not be verified.

Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
---
M docs/design-docs/cfile.md
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/crc.cc
M src/kudu/util/crc.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache.cc
19 files changed, 792 insertions(+), 51 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [rpc] reopen outbound connections mode for reactor

2017-04-26 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> There are a couple of issues I have with modes.
Well, most of our tests either have run-time parameters different from what 
they are by default in production, or turn on some non-production modes (e.g. 
fault injection and alike) to exercise some particular code paths for better 
coverage.  And the follow-up tests are not exception.  We could look at this 
new option as the boundary case of the RPC keepalive-related parameters -- in 
the real production, connections are closed after some period of inactivity (65 
seconds by default).

The regular long-lived connection scenarios are not in the scope of coming new 
tests: the authn token is verified only when connection is established, as I 
understand.  Those long-lived scenarios cannot be addressed in the scope of our 
integration tests because they cannot run for too long without being killed by 
the test framework.  If you think it's necessary to address those and 
specifically target some possible issues you know about, let's open a separate 
JIRA item for that.

Yes, combination of different modes might give a huge matrix of different 
behaviors to test and verify.  However, adding a multitude of 
change-of-behavior points in the client code is not any better: it would not be 
like in the production code anyway, and it would change the behavior in many 
places, so variation matrix would be of much greater dimensions.  As I see it, 
if not doing multiple incisions, current client code does not allow to 
implement a clean cut, driven-from-the-test-only scenario which would give good 
coverage for authn token verification paths.  If not touching the production 
code, then it would be necessary to implement a massive mock proxy for the 
client.  The latter wouldn't give any assurance that it would be close to the 
behavior of the real production code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [docs] Refresh and augment the known issues

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [docs] Refresh and augment the known issues
..


[docs] Refresh and augment the known issues

We've learned a lot about Kudu since people have started using it.
I've gathered in this patch what I think should be the new recommendations
we make to users.

Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Reviewed-on: http://gerrit.cloudera.org:8080/6699
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M docs/developing.adoc
M docs/installation.adoc
M docs/known_issues.adoc
M docs/kudu_impala_integration.adoc
4 files changed, 107 insertions(+), 21 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [docs] Refresh and augment the known issues

2017-04-26 Thread Jean-Daniel Cryans (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: [docs] Refresh and augment the known issues
..

[docs] Refresh and augment the known issues

We've learned a lot about Kudu since people have started using it.
I've gathered in this patch what I think should be the new recommendations
we make to users.

Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
---
M docs/developing.adoc
M docs/installation.adoc
M docs/known_issues.adoc
M docs/kudu_impala_integration.adoc
4 files changed, 107 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [docs] Refresh and augment the known issues

2017-04-26 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

Change subject: [docs] Refresh and augment the known issues
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6699/4/docs/developing.adoc
File docs/developing.adoc:

Line 167: - `NULL`, `NOT NULL`, `<>`, `OR`, `LIKE`, and `IN` predicates are not 
pushed to
This needs to be updated, `IN`, `NOT NULL`, `NULL`, can be taken out.  And some 
`LIKE` queries are also pushed down now (LIKE "foo%")


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1982 Java client calls NetworkInterface.getByInetAddress too often NetworkInterface.getByInetAddress (which is used by NetUtil.isLocalAddress) is very slow on Windows, so cache added.

2017-04-26 Thread Pavel Martynov (Code Review) 
Pavel Martynov has uploaded a new change for review.

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

Change subject: KUDU-1982 Java client calls NetworkInterface.getByInetAddress 
too often NetworkInterface.getByInetAddress (which is used by 
NetUtil.isLocalAddress) is very slow on Windows, so cache added.
..

KUDU-1982 Java client calls NetworkInterface.getByInetAddress too often
NetworkInterface.getByInetAddress (which is used by NetUtil.isLocalAddress) is 
very slow on Windows, so cache added.

Change-Id: I8fc9d40ebffa17ef43973dc23237dd31dd309c06
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
2 files changed, 33 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8fc9d40ebffa17ef43973dc23237dd31dd309c06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Pavel Martynov