date:20170815

Grant Henke has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 7:

I removed the Spark 2.2 upgrade since I found it only supports Java 8. I will 
handle that separately since it will require more change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Update outdated dependencies

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [java] Update outdated dependencies
..

[java] Update outdated dependencies

Includes the following version updates:
- Avro 1.8.1 -> 1.8.2
- Flume 1.6.0 -> 1.7.0
- Gradle 4.0.1 -> 4.1.0
- Jepsen 0.1.3 -> 0.1.5
- Many build plugin versions

Also moves and sorts the versions alphabetically.

Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
---
M java/gradle/buildscript.gradle
M java/gradle/dependencies.gradle
M java/gradle/wrapper/gradle-wrapper.properties
M java/gradlew
M java/kudu-flume-sink/pom.xml
M java/kudu-jepsen/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
9 files changed, 42 insertions(+), 38 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] rpc: move ConnectionId to its own file

Sailesh Mukil has posted comments on this change.

Change subject: rpc: move ConnectionId to its own file
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7685/1/src/kudu/rpc/proxy.h
File src/kudu/rpc/proxy.h:

PS1, Line 26: #include "kudu/rpc/outbound_call.h"
still needed?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[kudu-CR] rpc: move ConnectionId to its own file

Alexey Serbin has posted comments on this change.

Change subject: rpc: move ConnectionId to its own file
..


Patch Set 1: Code-Review+2

(1 comment)

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

PS1, Line 10: which didn't
nit: which _it_ didn't  ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out after transmission
of the request has begun but not finished, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This has caused some life cycle
issues with row batches in Impala before (e.g. IMPALA-5093).

This change fixes the problem above by modifying the RPC protocol
to allow a mid-transmission RPC call to be cancelled. Specifically,
a new footer is added to all outbound call requests. It contains
a flag, when true, indicates the outbound call was cancelled after
it has started but before it finished. The server will inspect this
flag and ignore inbound calls with this flag set. This footer enables
the caller to relinquish references to the sidecars early when an
outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the server always ignores
cancelled call requests, it's okay to send random bytes.

To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS
is introduced in this change. During connection negotiation, the
client will always include this flag in its feature set. A server which
supports parsing footer will include REQUEST_FOOTERS in its feature set
if it sees the client also supports it. A client will only send the
footer if the remote server has the RPC feature flag REQUEST_FOOTERS.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
19 files changed, 526 insertions(+), 160 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251: } else {
> yea I think that is actually reasonable. I'd do CHECK though, since this is
Done


PS2, Line 268: 
 :   // The timeout timer is
> per above, this is still best-effort in the case that you are talking to a 
Comments updated.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 199: etes.
> Comment rephrased.
Actually, comment removed.


PS2, Line 201:  the negotiation thread 
> ah, I see... negotiation_complete_ is set by the reactor thread itself wher
Yes, added a small note about this in the code.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

PS2, Line 352:   // behavior is a lot more efficient if memory is freed from 
the same thread
 :   // which allocated it -- this le
> looking at the implementation of faststring::release() I think these calls 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] disk failure: add persistent disk states

2017-08-15 Thread Andrew Wong (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: disk failure: add persistent disk states
..

disk failure: add persistent disk states

This patch adds a new version of the path set so failed disks are not
used the next time the server is run. This is needed, as data on a
failed disk may be partially written and thus should not be used.

To accomplish this, disk states, path names, and a timestamp are added
to the path set. Additionally, if any disks are failed when loading
instances from disk, an 'unhealthy' instance with no path set metadata
will be created instead of failing the startup process.

CheckIntegrity() is updated to accomodate this. Rather than
comparing all instances against an agreed-upon set of UUIDs, the single
path set with the largest timestamp is used as the main one to compare
against (if only old path sets are available, the first healthy one is
used). If no instances are healthy, CheckIntegrity() will fail, as all
disks are failed.

The main path set is checked to ensure its integrity (e.g. no
duplicates), after which it is upgraded with the extra metadata if
needed. It is then checked to ensure it is consistent with the healthy
instances (e.g. having the right UUIDs and paths).

Testing is done in a new iteration of CheckIntegrity(). Further testing
is done in data_dirs-test to ensure the directory manager can be opened
with failed disks.

Some notes:
- In the case of a server restart where all healthy disks fail to start
up and some known failed disks start working again, the server will
successfully start up with the bad disks (may have partially-written
data).
- If there are any unhealthy instances when upgrading the path sets (i.e.
adding disk states, paths, timestamp), a complete mapping of UUIDs to
paths will not be available, and CheckIntegrity() will fail.
- The main path set's disk states are updated to reflect the failure of
the instances. Since data on a failed disk cannot be trusted, disks
that are successfully read from but are already marked FAILED by the
main path set are not marked HEALTHY.

This patch is a part of a series of patches to handle disk failures. To
see how this fits, see 2.6 in:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit?usp=sharing

Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/fs.proto
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
9 files changed, 958 insertions(+), 197 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/13
--
To view, visit http://gerrit.cloudera.org:8080/7270
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC 
proxies
..


Patch Set 1:

@Sailesh, I think this may be sufficient to fix the 'rdns=false' issue in 
Impala. Note that you'll need to patch Impala to update to the new API upon 
pulling this KRPC patch in, though.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: some small cleanup in ConnectionId

Hello Sailesh Mukil, Alexey Serbin,

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

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

to review the following change.

Change subject: rpc: some small cleanup in ConnectionId
..

rpc: some small cleanup in ConnectionId

Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
3 files changed, 10 insertions(+), 33 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil

[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

Hello Sailesh Mukil, Alexey Serbin,

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

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

to review the following change.

Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC 
proxies
..

KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

This modifies the constructor of RPC proxies (generated and otherwise)
to take the remote hostname in addition to the existing resolved
Sockaddr parameter. The hostname is then passed into the ConnectionId
object, and plumbed through to the SASL client in place of the IP
address that was used previously.

The patch changes all of the construction sites of Proxy to fit the new
interface. In most of the test cases, we don't have real hostnames, so
we just use the dotted-decimal string form of the remote Sockaddr, which
matches the existing behavior.

In the real call sites, we have actual host names typically specified by
the user, and in those cases we'll need to pass those into the proxy. In
a few cases, they were conveniently available in the same function that
creates the proxy. In others, they are relatively far away, so this
patch just uses the dotted-decimal string and leaves TODOs.

In the case that Kerberos is not configured, this change should have no
effect since the hostname is ignored by SASL "plain". In the case that
Kerberos is configured with 'rdns=true', they also have no effect,
because the krb5 library will resolve and reverse the hostname from the
IP as it did before. In the case that 'rdns=false', this moves us one
step closer to fixing KUDU-2032 by getting a hostname into the SASL
library.

I verified that, if I set 'rdns = false' on a Kerberized client, I'm now
able to run  'kudu master status ' successfully where it would not
before. This tool uses a direct proxy instantiation where the hostname
was easy to plumb in. 'kudu table list ' still does not work because
it uses the client, which wasn't convenient to plumb quite yet.

Given that this makes incremental improvement towards fixing the issue
without any regression, and is already a fairly wide patch, I hope to
commit this and then address the remaining plumbing in a separate patch.

Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
---
M src/kudu/client/client-internal.cc
M src/kudu/client/master_rpc.cc
M src/kudu/client/meta_cache.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/rpc/connection.h
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
42 files changed, 231 insertions(+), 131 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil

[kudu-CR] rpc: move ConnectionId to its own file

Hello Sailesh Mukil, Alexey Serbin,

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

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

to review the following change.

Change subject: rpc: move ConnectionId to its own file
..

rpc: move ConnectionId to its own file

This class was previously implemented inside of outbound_call.{cc,h}
which didn't quite belong. In the several years since the code was
initially written we've moved more towards a "single class per header"
model unless the classes are truly trivial or really tightly coupled.
Neither is the case here.

Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/connection_id.cc
A src/kudu/rpc/connection_id.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc_context.cc
8 files changed, 174 insertions(+), 119 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil

[kudu-CR] gutil: remove use of deprecated headers

Hello Sailesh Mukil, Alexey Serbin,

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

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

to review the following change.

Change subject: gutil: remove use of deprecated headers
..

gutil: remove use of deprecated headers

We've been on C++11 for a long time now. This commit removes usage of
ext/hash_map and ext/hash_set headers in favor of the now-standardized
unordered_map and unordered_set.

Additionally, it removes our specializations of __gnucxx::hash<> and
instead changes to specializing std::hash<> where appropriate. Some of
those specializations are now part of the standard and have been removed
as necessary to fix compilation.

This does not yet remove the -Wno-deprecated setting in the top-level
CMakeLists.txt, since it appears we have a lot of usage of our own
internal deprecated functions.

Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
---
M CMakeLists.txt
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/hash/hash.cc
M src/kudu/gutil/hash/hash.h
M src/kudu/gutil/strings/serialize.cc
M src/kudu/gutil/strings/serialize.h
M src/kudu/gutil/strings/split.cc
M src/kudu/gutil/strings/split.h
9 files changed, 40 insertions(+), 109 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil

[kudu-CR] separate DataDirManager from BlockManagers

2017-08-15 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 7:

(8 comments)

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

Line 146: bm_.reset();
Why does the block manager have to be destroyed separately up here? Why isn't 
the reset on L154 sufficient?

Oh, in log_block_manager-test you have a comment explaining this. Can you add 
it here too?


http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

Line 289:   LOG(INFO) << "Constructing a new DataDirManager";
Still need this?


Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> warning: passing result of std::move() as a const reference argument; no mo
Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. And 
it doesn't really make sense to change that, since DataDirMetrics doesn't take 
a ref itself. But, storing the MetricEntity in 'opts' by value makes sense, so 
just dropping the std::move() here should be sufficient.


PS7, Line 339:   canonicalized_data_roots.insert(canonicalized_data_roots.end(),
 :   
canonicalized_data_roots_set.begin(),
 :   
canonicalized_data_roots_set.end());
Isn't this logically the same as the returned values from  the calls to 
FsManager::CanonicalizePath? What's the purpose of all of the logic in between?


PS7, Line 350:   const int kFlagCreateTestHolePunch = 0x1;
 :   const int kFlagCreateFsync = 0x2;
But why bother with flags at all? Flags are a means of defining an API and 
there's no API here. That is, where you have "flags & kFlagCreateFsync" you 
could just have "true". And where you have "flags & kFlagCreateTestHolePunch" 
you could just have "block_manager_type_ == "log"".


http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS6, Line 229:   // Shuts down all directories' thread pools.
 :   void Shutdown();
 : 
 :   // Waits on all directories' thread pools.
 :   void WaitOnClosures();
 : 
 :   // Initializes the data directories on disk.
 :   //
 :   // Returns an error if initialized directories already exist.
 :   Status Create();
 : 
 :   // Opens existing data roots from disk and indexes the files 
found.
 :   //
> I whittled it down to Init(), Create(), and Open(). The logic in Init() (ca
Would it be possible to convert the static Init and the Create and/or Open into 
just "static CreateNew" and "static LoadExisting"? That would be simpler, IMHO. 
Couldn't the canonicalization be called privately by CreateNew/LoadExisting? I 
get that both FsManager and BlockManager hew to this "call Create() AND Open() 
when creating a new thing, call just Open() to open an existing thing" pattern, 
but it's pretty confusing and inconsistent with other objects in the Kudu 
codebase.

Also, Init() isn't a great name for what is now a factory method, since 
elsewhere we use Init() for "initialize more state in this existing object". 
Better to use Create() for that.


http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS7, Line 200: Defaultas to fales
Defaults to false

Also nit: having this be on a new line (like L197) would be more consistent 
with how you've done metric_entity.


Line 378:   // The canonicalized roots provided at construction time, taken 
verbatim.
I understand what this means, but "construction time" is a little weird because 
it doesn't refer to the time that the user constructed the object (i.e. at 
Init() time, when the roots were _not yet_ canonicalized), but to the time that 
the private constructor was called by Init(). Might be less confusing to reword.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


Patch Set 4: Code-Review+2 Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.

STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this
STACK_OF(X509) object.

When we call AddTrustedCertificate(Cert&), we iterate through the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.

Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain
certificates, if we want to use IPKI with chain certificates,
additional functionality will need to be added with clearer APIs.

External PKI makes use of this ability to add a chain CA if necessary.

Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.

Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Reviewed-on: http://gerrit.cloudera.org:8080/7662
Reviewed-by: Todd Lipcon 
Tested-by: Todd Lipcon 
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 431 insertions(+), 55 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

2017-08-15 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 10:

(5 comments)

Hao, Dan, and I had a long discussion about this patch, and I wanted to 
reproduce our conclusions here for everyone else:

1. The cfile and block performance-related knobs are useful for testing, but 
are not as important as having a good API and a robust implementation. IIUC, 
the resplitting of FlushDataAsync and Finalize was due to retaining all the 
existing flags; let's not do this. Let's stick to the earlier approach (where 
Finalize implies a flush), and adjust the flags if need be.

2. What flags are worth preserving? We identified the need for just one, which 
dictates when a block's data should be flushed. It has three values: "finalize" 
(default value), "close" (defers the flush to when the entire transaction is 
committed), and "none" (doesn't flush at all, "flushing" will happen when the 
transaction commits and files are fsynced).

3. When should AppendMetadata be called? One option is to do it at Finalize 
time. Another is to defer it to Close. The problem with doing it during 
Finalize is that it exacerbates the "1. AppendData, 2. AppendMetadata, 3. 
SyncData, 4. SyncMetadata" race: if we crash between #2 and #3, we may end up 
with just the metadata on disk, and if we AppendMetadata during Finalize, we 
have many more blocks' worth of metadata that can land on disk without 
corresponding data.

4. But AppendMetadata is called during Close, we may end up with out-of-order 
metadata entries on disk, since transactions could commit out of order. Do we 
care? We don't think so; the LBM implementation should already be robust to 
this. An alternative is to use an in-memory buffer to retain metadata order, 
but this didn't seem worth the complexity.

5. What to do about zero length blocks? Should their metadata be written, or 
skipped? The answer is: it must be written, or attempts to Close such blocks 
should fail. If for whatever reason we skipped the metadata append, we could 
end up with block IDs whose blocks can't be found on startup.

http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

Line 423: TEST_P(TestCFileBothCacheTypes, TestWrite100MFileInts) {
You should be able to use this to replace TestCFileBothCacheTypes. Here's what 
I think you'll need to do:

1. Rewrite TestCFileBothCacheTypes to be an empty subclass of 
TestCFileBothCacheCloseTypes.
2. The first call to INSTANTIATE_TEST_CASE_P should be for 
TestCFileBothCacheTypes and its values should be two CFileDescriptors that vary 
the CacheType but both use the CLOSE CloseType.
3. The second call to INSTANTIATE_TEST_CASE_P should be for 
TestCFileBothCacheCloseTypes and should pass all four CFileDescriptors.

After our long in-person discussion about other changes to this patch, it's 
possible all of this becomes moot because there's no longer a cfile flag to 
test.


Line 449: 
If the default value of this gflag changes to true, we'll never test the false 
case. You should use a switch statement and explicitly set it to either true or 
false.

After our long in-person discussion about other changes to this patch, it's 
possible all of this becomes moot because there's no longer a cfile flag to 
test.


http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

PS16, Line 59: // - users could always change this to "close", which slows down 
throughput
 : //   but may improve write latency.
This part isn't quite right anymore. Should the entire comment be rewritten in 
light of the split into two gflags?

After our long in-person discussion about other changes to this patch, it's 
possible this becomes moot because there's no longer a cfile flag. Though you 
may still need to move this explanation (and rewrite it).


PS16, Line 63:  
Nit: got an extra space here.


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

Line 1288:   // Ensure the same container has not been marked as available 
twice.
> I was trying to check in the available_containers_by_data_dir_ map, there i
Ah, I forgot that available_containers_by_data_dir_::value_type is a vector. So 
indeed, if we marked the container as available twice, you'd see two entries in 
that vector. Makes sense.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert

[kudu-CR] separate DataDirManager from BlockManagers

2017-08-15 Thread Andrew Wong (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: separate DataDirManager from BlockManagers
..

separate DataDirManager from BlockManagers

Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.

Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager's new
  public initializer Init().
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
  instead of shutting down the DataDirManager as to not directly affect
  the DataDirManager

To clarify some vocabulary:
- Data root: a top-level directory specified by 'fs_data_dirs'
- Data dir: a subdirectory of a data root, named 'data'. Blocks are
  placed in this directory.

Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 473 insertions(+), 349 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] separate DataDirManager from BlockManagers

2017-08-15 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 6:

(34 comments)

http://gerrit.cloudera.org:8080/#/c/7602/6//COMMIT_MSG
Commit Message:

PS6, Line 23: directory
> directly
Done


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

PS6, Line 205:   // Pass in a report to prevent the block manager from logging
 :   // unnecessarily.
 :   FsReport report;
> Nit: move this down to just below Open().
Done


Line 691: ASSERT_OK(env_util::CreateDirIfMissing(this->env_, paths[i]));
> Why not a simple CreateDir() here and below? Under what circumstances would
These shouldn't have been created so it should be safe to just create.


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

PS6, Line 260:   // Exposes the underlying DataDirManager.
 :   virtual DataDirManager* dd_manager() = 0;
> Why does this still exist? Shouldn't callers outside the block managers get
Had been used in tests, but those tests can use the dd_manager directly; 
removed.


http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:

Line 65: LOG(INFO) << GetDirNames(kNumDirs)[0];
> Still want this?
Done


Line 77:   CHECK_OK(env_util::CreateDirIfMissing(env_, ret[i]));
> Why not a simpler CreateDir?
Done


http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

Line 90: TAG_FLAG(fs_lock_data_dirs, unsafe);
> Also tag as 'evolving', for completeness.
Done


Line 272: const char* kInvalidPath = "";
> Where is this used?
Future patch. Eventually we'll need something stored when the directories fail 
to canonicalize. This isn't it, but it was a precursor to what's implemented 
later on. Removed.


PS6, Line 288:   if (metric_entity) {
 : metrics_.reset(new DataDirMetrics(metric_entity));
 :   }
> You changed metric_entity to be pass-by-cref because it's not being moved h
Done


Line 334:   data_root_map_.swap(data_root_map);
> Maybe write to the member at the end of the function, after all the interme
Done


Line 340: if (!canonicalized_data_fs_root.empty()) {
> When is this empty?
Here and elsewhere you saw traces of an implementation where things that failed 
to canonicalize were given an invalid "" path name. This has changed, and this 
should be removed (particularly since this patch is independent of disk 
failures).


Line 372: // Ensure the data dirs exist and create the instance files.
> IMHO this comment was better placed just before the for loop, so it's clear
Done


Line 375: // In test, this is the input path, IRL, this is the paths with 
kDataDirName
> Please reword; not exactly sure what "input path" means, this should be mor
Yeah, this is a pretty dumb comment and shouldn't have been in this patch.


Line 526:   group_by_tablet_map_.clear();
> Isn't this empty at Open() time anyway? Why bother?
In tests, I found it convenient to have calls to Open() be idempotent, i.e. 
calling Open() multiple times would create leave the directory manager in the 
same state each time. Given the reset behavior of Init(), this should always be 
the case even without this.


http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS6, Line 195:   static const int kFlagCreateTestHolePunch = 0x1;
 :   static const int kFlagCreateFsync = 0x2;
> These were only used in Create(), so they needn't exist at all anymore.
Done


Line 198:   static const char* kDataDirName;
> Why does this need to be public? Doc it.
Was only used publicly in tests. Made private.


Line 220:  AccessMode mode = AccessMode::READ_WRITE);
> Can we get by without the default value? There aren't that many constructor
Done.

It's used in a handful of tests, but we needn't make that an issue in 
production code.


Line 240:   // max allowed, or if 'mode' is MANDATORY and locks could not be 
taken.
> 'mode' refers to a parameter that no longer exists.
Done


PS6, Line 229:   // Initializes, sanitizes, and canonicalizes the directories.
 :   Status Init();
 : 
 :   // Initializes the data directories on disk.
 :   //
 :   // Returns an error if initialized directories already exist.
 :   Status Create();
 : 
 :   // Opens existing data roots from disk and indexes the files 
found.
 :   //
 :   // Returns an error if the number of on-disk directories found 
exceeds the
 :   // max allowed, or if 'mode' is MANDATORY and locks could not 
be taken.
 :   Status Open();
> Normally we try to provide at most one, sometimes two functions with which

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


Patch Set 4:

> Build Failed
 > 
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/9318/ : FAILURE

The test failure looks like a flaky test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [iwyu] first pass

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] first pass
..


Patch Set 15:

> definitely woudl be nice to get rid of some of the more common
 > pragmas... here's a count from grep | uniq -c | sort -nk1
 > 
 > 10 #include   // IWYU pragma: keep
 > 15 // IWYU pragma: no_include 
 > 16 // IWYU pragma: no_include 
 > 26 #include  // IWYU pragma: keep
 > 27 // IWYU pragma: no_include 
 > 32 // IWYU pragma: no_include "kudu/util/logging.h"
 > 37 #include  // IWYU pragma: keep
 > 
 > These are all from IWYU bugs? Are they known bugs? If we are going
 > to expect people to keep IWYU clean at precommit I wish it were a
 > bit less error-prone, otherwise the extra time spent on trying to
 > make it happy will outweigh any extra time saved with faster
 > builds, no?

The issue with logging.h headers is resolved.  I would attribute it to a bug in 
IWYU, but I worked it around using custom mappings for some symbols in the glog 
library.

I think I can address the rest of non-boost pragmas.  For the  and 
 I can just follow the recommendations from the tool:  the 
 is needed every time  is included and the 'ostream& 
ostream::operator <<(...)' is used, like LOG(INFO) << "Hello world";

The  requires deeper examination, but seems to be doable as 
well.

The boost-related pragmas would require updating the existing boost mappings -- 
I will take care of that as well.

 > 
 > Or is the plan to just periodically (eg once a month) run this and
 > fix any errors it comes up with, and hopefully those incremental
 > fixes will be smaller than this initial patch?

As I understand, the idea was to have it as a pre-commit check.  Yes, in that 
case it's necessary to complete the clean-up of those warnings.  I'll try to do 
that today.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> I just did a grep in the Kudu code. There doesn't seem to be any use of Rpc
yea I think that is actually reasonable. I'd do CHECK though, since this is 
rare so we don't care about perf, and I'd rather have an obvious crash than a 
weird memory corruption


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> Thanks for summarizing.
I just did a grep in the Kudu code. There doesn't seem to be any use of 
RpcController::AddOutboundSidecar(). I suppose if Kudu starts using client 
sidecar for some new functionality, it needs to update the Kudu server code to 
make use of the sidecars too so mixed version doesn't really apply in this case.

May be it's safe to just add a DCHECK to verify that the sidecars' bytes are 
zero if remote doesn't support footers or will it cause any confusion ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


Patch Set 4:

(10 comments)

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

PS2, Line 15: 
> nit: please make the string <= 72 characters long
Done


PS2, Line 18: through 
> nit: typo
Done


PS2, Line 25: 
> nit: please make the string <= 72 characters long
Done


http://gerrit.cloudera.org:8080/#/c/7662/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

Line 149:   auto cleanup = MakeScopedCleanup([&]() {
> in the case of a bad PEM file format, I think this would end up leaving a p
Great point. I removed SCOPED_OPENSSL_NO_PENDING_ERRORS here and below.


Line 156:   // Iterate through the chain certificate and add each one to the 
stack.
> similar to above, I think this will result in returning null but with no pe
Agreed. I moved this to the calls Cert::FromString() and Cert::FromFile().

A test for this already exists in CertTest.CertInvalidInput, which works even 
after moving this from here.


PS3, Line 165: 
> the free call is no longer below. maybe just say 'the ScopedCleanup'
Done


Line 175: if (ret <= 0) return ret;
> same
Done


Line 188:   }
> same
Done


Line 202: 
> same
Done


Line 212:   }
> same
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..

KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.

STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this
STACK_OF(X509) object.

When we call AddTrustedCertificate(Cert&), we iterate through the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.

Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain
certificates, if we want to use IPKI with chain certificates,
additional functionality will need to be added with clearer APIs.

External PKI makes use of this ability to add a chain CA if necessary.

Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.

Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 431 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> hm... to summarize the possibilities here:
Thanks for summarizing.

#3 seems attractive but I am a bit worried about the arbitrarily large sidecars 
which we have to buffer. It may end up pushing Impalad over the process memory 
limit and getting it OOM killed.

For all practical purposes in Impala, we envision the first version of Impala 
using KRPC will have both server and client supporting footers. #2 is not ideal 
for mixed cluster usage with client sidecars but arguably it's not a regression 
either. I don't know how common the usage of client sidecars in Kudu today is. 
If Impala is the only use case, may be it's safer to stick with #2 for now.

Yet another idea is to fabricate certain byte sequence which will always fail 
to deserialize in the remote server but I don't think it will work once the 
request PB has been sent so I won't bet on it.

Please let me know what you think.


PS2, Line 703:if (!transfer->TransferStarted()) {
 :   if (transfer->is_for_outbound_call()) {
 : if (!StartCallTransfer(transfer)) {
> these can all be collapsed into a single boolean expression now, right?
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 199: takes
> typo: take
Comment rephrased.


PS2, Line 199: Callers should takes this into account.
> not quite sure what this sentence is indicating. Don't we assume that calle
Rephrased the comment. It intends to point out that this function will return 
false before negotiation completes so callers (in particular reactor threads) 
need to handle it properly by doing the "right thing" (whatever that means for 
the caller).


PS2, Line 284: sending it for
 :   // the first time
> I think better to say "beginning to send it" or "beginning transmission" or
Done


Line 288:   // If the checks pass, the outbound call will be set to 'SENDING' 
state.
> if the checks don't pass, is this responsible for 'finishing' the call? eg 
Comments added.


PS2, Line 289: Append
> nit: "Appends"
Done


PS2, Line 303:  if the transfer is for an outbound call.
> not quite clear here which part of the sentence the "if ..." applies to. If
Thanks for the suggestion. Done.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 29: #include "kudu/rpc/outbound_call.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


Line 30: #include "kudu/rpc/connection.h"
> fix sort order
Done


PS2, Line 260: shutted
> nit: "shut"
Done


Line 266:   // fall-through if remote supports parsing footer.
> we have a special macro here 'FALLTHROUGH_INTENDED' which turns into someth
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS2, Line 159: send_footer
> is this out of date? I don't see this param
Done


Line 171:   // to indicate that the call has been aborted.
> worth indicating whether this call requires that AppendFooter() has previou
Done


PS2, Line 210: IsBeingSent
> Lol. I considered using IsOnTransferQueue() at some point.
Actually, I now recall why I ruled against using that name as it can easily 
mislead readers into believing that the call is in ON_OUTBOUND_QUEUE state. 
Renamed to IsInTransmission() ?


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

PS2, Line 274: Note that the footer is designed to be modified
 :   // after the initial serialization and it will be 
re-serialized after modification.
 :   // To avoid unexpected change in the total message size, keep 
to using fixed sized
 :   // fields only in the footer.
> move this comment to be below the 'aborted' field, since it documents any n
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/serialization.h
File src/kudu/rpc/serialization.h:

Line 78: void IncrementMsgLength(size_t inc_len,
> warning: function 'kudu::rpc::serialization::IncrementMsgLength' has a defi
Done


Line 78: void IncrementMsgLength(size_t inc_len,
> please fix this nit
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

Line 78: /* Initialize the dummy buffer with a known pattern for easier 
debugging. */
> style: // C++ style comment
Done


Line 190:   ++n_payload_slices_;
> how about a post-increment on the prior line?
Done


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

[kudu-CR] WIP: [iwyu] first pass

Todd Lipcon has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 14:

definitely woudl be nice to get rid of some of the more common pragmas... 
here's a count from grep | uniq -c | sort -nk1

 10 #include   // IWYU pragma: keep
 15 // IWYU pragma: no_include 
 16 // IWYU pragma: no_include 
 26 #include  // IWYU pragma: keep
 27 // IWYU pragma: no_include 
 32 // IWYU pragma: no_include "kudu/util/logging.h"
 37 #include  // IWYU pragma: keep

These are all from IWYU bugs? Are they known bugs? If we are going to expect 
people to keep IWYU clean at precommit I wish it were a bit less error-prone, 
otherwise the extra time spent on trying to make it happy will outweigh any 
extra time saved with faster builds, no?

Or is the plan to just periodically (eg once a month) run this and fix any 
errors it comes up with, and hopefully those incremental fixes will be smaller 
than this initial patch?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7662/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

Line 149:   if (!info) return nullptr;
in the case of a bad PEM file format, I think this would end up leaving a 
pending OpenSSL error, and then the SCOPED_OPENSSL_NO_PENDING_ERRORS would 
crash.

I'm not convinced that the SCOPED_OPENSSL_NO_PENDING_ERRORS is appropriate 
here, since this is function is supposed to have an SSL-like calling convention 
(return NULL and leave a pending error on failure). The pending error should be 
handled (converted to Status) by the code in FromBIO(...) in openssl_util_bio.h.


Line 156:   if (chain_len == 0) return nullptr;
similar to above, I think this will result in returning null but with no 
pending errors on the OpenSSL error stack. So when FromBIO(...) calls 
GetOpenSSLErrors() it's going to get an empty string and this will then return 
a RuntimeError with no explanation.

Perhaps it's better to remove this check and change the check to be in the code 
which consumes the STACK_OF(X509) (ie Cert::FromBIO)


PS3, Line 165: free() call below
the free call is no longer below. maybe just say 'the ScopedCleanup'


Line 175:   SCOPED_OPENSSL_NO_PENDING_ERRORS;
same


Line 188:   SCOPED_OPENSSL_NO_PENDING_ERRORS;
same


Line 202:   SCOPED_OPENSSL_NO_PENDING_ERRORS;
same


Line 212:   SCOPED_OPENSSL_NO_PENDING_ERRORS;
same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> Good point. We could also reach this point because negotiation has not comp
hm... to summarize the possibilities here:

- on caller-requested cancellation:
-- if pre-transmission or post-transmission, immediately call callback with 
Aborted case
-- if mid-transmission:
--- if footers are negotiated, do the footer magic and immediately call callback
--- else: just treat as a no-op (this case isn't really expected so it's OK to 
be best-effort)

- on timeout:
-- if pre-transmission or post-transmission, call callback
-- if mid-transmission and sidecars are not in use, or sidecars in use but 
footers are supported, abort and call callback
-- if mid-transmission and sidecars are in use, but footers are not enabled, 
then we are in the sticky situation described above.

The choices in this sticky situation are:

1) "delay the timeout" -- as you suggested above, set some flag, and then call 
the timeout callback when the transmission is complete. Unfortunately that 
could be arbitrarily long into the future, and Kudu at least would not be very 
happy with that.

2) "call the timeout anyway" -- which risks use-after-free of the sidecar in 
the Impala use case (i.e this is today's buggy behavior)

3) "copy the remaining sidecar and call immediately" - what if we actually just 
allocated a new buffer, copied the remaining data into it, and relocated the 
slices to point to it instead of the original data? We have some untracked 
memory which is ugly, but we avoid the use-after-free, and we don't expect this 
situation to really happen in practice since it only would happen in a 
mixed-version cluster which is also using client sidecars.

Any thoughts on choice #3?

hm... one thing I am a little afraid of in the timeout case is having a 
regression on timeout "timeliness" in the existing Kudu use case. If we set a 1 
second timeout, we really expect it to return in one second even if we are 
stuck sending.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 201: negotiation_complete_ &&
> I need this to avoid a TSAN warning about concurrent access to remote_featu
ah, I see... negotiation_complete_ is set by the reactor thread itself whereas 
remote_features_ is directly mutated by the negotiator. Good job TSAN! this 
would have been a nasty bug.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

Line 223:   // the transfer if only the footer is left.
> Actually, in this case, we will still invoke OutboundCall::Cancel() right a
ah, I see, that's right.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty]: added include-what-you-use

Alexey Serbin has submitted this change and it was merged.

Change subject: [thirdparty]: added include-what-you-use
..


[thirdparty]: added include-what-you-use

Build the include-what-you-use utility along with the LLVM toolchain.

The project URL:
  https://include-what-you-use.org/

Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Reviewed-on: http://gerrit.cloudera.org:8080/7593
Reviewed-by: Adar Dembo 
Tested-by: Alexey Serbin 
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-add-iwyu.patch
A thirdparty/patches/llvm-iwyu-include-picker.patch
A thirdparty/patches/llvm-iwyu-nocurses.patch
M thirdparty/vars.sh
5 files changed, 39 insertions(+), 2 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Alexey Serbin: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [build-support] added IWYU filter script

Alexey Serbin has submitted this change and it was merged.

Change subject: [build-support] added IWYU filter script
..


[build-support] added IWYU filter script

Added a script to filter the output from the include-what-you-use tool.
The idea is that the IWYU tool v0.8 is of alpha quality and produces
many incorrect recommendations.  Most of those can be addressed by using
various 'IWYU pragma' directives, but not in the case of auto-generated
files.  Adding those directives is a very tedious job, and only
some of those places are addressed.  Moreover, the incorrect IWYU
recommendations seem to be a little bit different version from version.

Also, added IWYU mappings for the boost library (imported from the IWYU
source tree) and the glog, gflags, gtest libraries.

Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa
Reviewed-on: http://gerrit.cloudera.org:8080/7604
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
A build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-all-private.imp
A build-support/iwyu/mappings/boost-all.imp
A build-support/iwyu/mappings/gflags.imp
A build-support/iwyu/mappings/glog.imp
A build-support/iwyu/mappings/gtest.imp
M build-support/release/rat_exclude_files.txt
7 files changed, 10,295 insertions(+), 0 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

2017-08-15 Thread Hao Hao (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that for LBM container can be reused without flushing
in-flight writable blocks to disk.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 734 insertions(+), 304 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-871. Support tombstoned voting

2017-08-15 Thread Edward Fancher (Code Review)

Edward Fancher has posted comments on this change.

Change subject: KUDU-871. Support tombstoned voting
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/integration-tests/tombstoned_voting-itest.cc
File src/kudu/integration-tests/tombstoned_voting-itest.cc:

Line 48: 
Replicated masters can have tombstoned replicas too, is that right? Does the 
tombstone voting apply to them? If so, would it be worthwhile to also have a 
test that includes tombstoned voting for a master tablet replica?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Edward Fancher 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] Update outdated dependencies

Grant Henke has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 6:

Turns out Spark 2.2 dropped java 7 support. I will open a discussion to see how 
we want to handle that on the mailing list. That would also allow an upgrade of 
Guava here.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Update outdated dependencies

2017-08-15 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Update outdated dependencies

Hello Kudu Jenkins,

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

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

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

Change subject: [java] Update outdated dependencies
..

[java] Update outdated dependencies

Includes the following version updates:
- Avro 1.8.1 -> 1.8.2
- Flume 1.6.0 -> 1.7.0
- Gradle 4.0.1 -> 4.1.0
- Jepsen 0.1.3 -> 0.1.5
- Spark 2.1.1 -> 2.2.0
- Many build plugin versions

Also moves and sorts the versions alphabetically.

Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
---
M java/gradle/buildscript.gradle
M java/gradle/dependencies.gradle
M java/gradle/wrapper/gradle-wrapper.properties
M java/gradlew
M java/kudu-flume-sink/pom.xml
M java/kudu-jepsen/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
9 files changed, 43 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(4 comments)

Thanks again for taking a look. Replies to some questions below before 
addressing the rest of the comments.

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> what about in the case of talking to an old server which doesn't support fo
Good point. We could also reach this point because negotiation has not 
completed yet. It seems always safe to always call Abort() if negotiation 
hasn't completed yet as we shouldn't have started sending anything yet. Once 
negotiation has completed and the remote doesn't support footer, yes, then this 
should be a no-op for cancellation.

It slightly more complicated for the time-out case if the remote doesn't 
support footer. We cannot invoke OutboundCall::SetTimedOut() right away in 
HandleOutboundCallTimeout() below if the call is mid-transmission or we will 
hit the original bug again. We probably need to leave a status or bool 
indicating that the call has timed out and in OutboundCall::SetSent(), we need 
to invoke the SetTimedOut() instead of waiting for the response.

Please let me know if you think the above proposal is too complicated.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 201: negotiation_complete_ &&
> is negotiation_complete_ necessary? before negotiation is complete, isn't r
I need this to avoid a TSAN warning about concurrent access to remote_features_ 
between the negotiation thread and the reactor thread.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS2, Line 210: IsBeingSent
> hrm, I actually am going to disagree with my previous comment and still thi
Lol. I considered using IsOnTransferQueue() at some point.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

Line 223:   // the transfer if only the footer is left.
> could this last bit result in a "hung" cancellation in the case that the se
Actually, in this case, we will still invoke OutboundCall::Cancel() right away 
in ReactorThread::CancelOutboundCall() after calling into this function via 
Connection::CancelOutboundCall().

It's true that the OutboundCall object may still hang around for a while 
because the TransferCallback still has a reference to it. Please let me know if 
I may have misunderstood your comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu