[kudu-CR] block manager: require file cache

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: block manager: require file cache
..


block manager: require file cache

The file cache has been in Kudu since 1.2 and we've yet to see any major
issues. So let's make it required; doing so lets us clean up some branches.

As for block_manager_max_open_files, I changed the meaning of the value 0
from "no limit" to "error"; this seemed net less disruptive than changing
the meaning of -1 from "40%" to "error" and 0 from "no limit" to "40%".

Change-Id: I9946e537e0b4abd66a9a90fc05df04232db68bc0
Reviewed-on: http://gerrit.cloudera.org:8080/6880
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/fs/block_manager.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 54 insertions(+), 100 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

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

[kudu-CR] KUDU-1549: compact LBM container metadata files at startup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1549: compact LBM container metadata files at startup
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio, 
0.10,
:   "Desired ratio of live block metadata in log 
containers. If a "
:   "container's live to total block ratio dips below 
this value, "
:   "the container's metadata file will be compacted at 
startup.");
> I can't say I have a strong opinion on the tagging given how often I hem an
looking at a data/ dir on a cluster that has been around for quite some time, 
most of the metadata files seem to be around 400KB. Assuming 100MB/sec 
sequential throughput and 10ms seek, it definitely seems like the startup time 
would be seek-dominated (10 or 20ms seek depending whether various internal 
metadata pages are hot in cache, plus only 4ms of sequential read time). 

Unfortunatley, if that's true, this might mean that this optimization won't 
yield much of an improvement in the case that the metadata files are cold after 
a full machine reboot. However, in a "warm reboot" where the metadata files are 
in cache, or in the case when the metadata is on SSD, it should be a good CPU 
savings.

I did a quick test on the above node (2000 metadata files) by calling 
drop_caches=3 and then timing 'cat *.metadata > /dev/null'. It took 2m14s. 
Interestingly, though, 'head --bytes=4 *.metadata > /dev/null' took only 54s. 
Looking at 'filefrag *.metadata' it seems most of the metadata files have ~15 
extents, so each file is probably doing way more than the expected number of 
seeks.

If the above fragmentation is normal (seems quite plausible given the files are 
written slowly over time, old ones are appended to, etc) then it seems like 
rewriting them should have a nice perf boost for future startups just by virtue 
of defragmenting the underlying FS (regardless of any actual compaction). Going 
as far as to use FIEMAP is probably overkill, but being aggressive to compact 
probably isn't crazy.

(Sorry for the long comment... curious your thoughts)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
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] log block manager: reopen container metadata writers at the OS level

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 4: Code-Review+1

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

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

[kudu-CR] WIP [rpc] introduced per-RPC credentials policy

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP [rpc] introduced per-RPC credentials policy
..


Patch Set 3:

(7 comments)

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

Line 7: WIP [rpc] introduced per-RPC credentials policy
nit: better to say "Introduce" instead of "Introduced" for consistency with 
most of our messages (see https://chris.beams.io/posts/git-commit/ point 5)


PS3, Line 23: but
other than


http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:

Line 386:   ++it;
Can you add a comment why not breaking here?


PS3, Line 411:   {
 : UserCredentials credentials;
 : 
credentials.set_real_user(conn_id.user_credentials().real_user());
 : credentials.set_authn_token((policy == 
CredentialsPolicy::PRIMARY_CREDENTIALS)
 : ? boost::none : reactor()->messenger()->authn_token());
 : (*conn)->set_local_user_credentials(std::move(credentials));
 :   }
There's a bit of a mess here, in that the Proxy object has 
set_user_credentials(), and UserCredentials purports to be an external-facing 
class, but here we're basically ignoring any token that the user might have 
tried to set in UserCredentials and instead overriding it with the token from 
the Messenger.

It appears that we basically don't use Proxy::set_user_credentials() anywhere, 
though. Perhaps it would be best to make it an internal-facing class for KRPC, 
so this is a bit less messy, given it's not used anyway?

Alternatively, maybe putting authn_token in UserCredentials isn't the right 
method, adn instead we should just pass it directly into the 
StartConnectionNegotiation call? It's only needed during negotiation, and once 
negotiated, we only need to remember how we authenticated the connection, not 
the actual token used, right?


Line 516: it = client_conns_.erase(it);
can you preserve the assertion here and ensure that we actually erase() exactly 
one connection in this loop?


http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

PS3, Line 51: on 
nit: "on a"


PS3, Line 59: username
we don't actually support username/password anymore, right?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2
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] block manager: require file cache

[Adar Dembo (Code Review)]

Hello Todd Lipcon,

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

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

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

Change subject: block manager: require file cache
..

block manager: require file cache

The file cache has been in Kudu since 1.2 and we've yet to see any major
issues. So let's make it required; doing so lets us clean up some branches.

As for block_manager_max_open_files, I changed the meaning of the value 0
from "no limit" to "error"; this seemed net less disruptive than changing
the meaning of -1 from "40%" to "error" and 0 from "no limit" to "40%".

Change-Id: I9946e537e0b4abd66a9a90fc05df04232db68bc0
---
M src/kudu/fs/block_manager.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 54 insertions(+), 100 deletions(-)


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

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

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

By popular demand, I've attached below a long explanation of Kudu's approach
to object initialization and thread safety that came up during code review.

Because we don't use exceptions within Kudu, the initialization of an object
is typically done in two phases:
 1. Constructor, for operations that cannot fail.
 2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status
type, and needn't be thread safe because it is called right after the
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new
object and one to initialize an object with existing state. The idea is that
callers always use the constructor and then call one of the two initializer
functions, depending on their use case. WritablePBContainerFile is one such
object; Init() is for a brand new container file, and Open() is for a
container file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer
function for files with existing state, and it was an arbitrary function
(like Append()) that could be called at any time and by any thread. That
second responsibility is why it was thread safe. Now, OpenExisting() is just
the equivalent of CreateNew(), and so it makes sense for it to be just as
thread unsafe as CreateNew() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the
LBM didn't _need_ it to be thread safe. But, I think it was net less
confusing for Reopen() to be thread safe (and thus equivalent in semantics
to Append()) than for it to be thread unsafe (and thus exceptional when
compared to Append(), Sync(), etc.).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 33 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add fault injection of EIOs

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add fault injection of EIOs
..


Patch Set 2:

(5 comments)

a small directed test would be nice. maybe just add a test to env_util test 
where you set the prob flag to 1 and make sure the methods you care about 
return what you want.

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

PS2, Line 129: the operation should attempt to fail without crashing
maybe "If false, EIOs are just cause an Status::IOError to be returned. In this 
case, it's up to the caller to decide how to handle the error."


Line 149: 
extra line


PS2, Line 150: env_inject_eio
any operation or just the ones that whose filenames match the regex below?


PS2, Line 283: WARNING
use ERROR instead?


Line 1651: 
spurious change


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] fs manager: optimize tmp file deletion

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: fs_manager: optimize tmp file deletion
..


Patch Set 2: Verified+1

The Python tests failed because a master couldn't start due to an "address 
already in use" error.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] fs manager: optimize tmp file deletion

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: fs_manager: optimize tmp file deletion
..


fs_manager: optimize tmp file deletion

In a run of dense_node-itest, the bulk of CPU time[1] was spent
canonicalizing paths while cleaning up temporary files. This patch optimizes
that in two ways:
- Stop canonicalizing paths. We already canonicalize the WAL and data dir
  roots; that's good enough for admin-provided symlinks.
- Split WAL and data root cleaning, parallelizing the latter through the
  DataDirManager.

1. Though admittedly the majority of wall clock time was waiting on IO.

Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Reviewed-on: http://gerrit.cloudera.org:8080/6837
Reviewed-by: Todd Lipcon 
Tested-by: Adar Dembo 
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
5 files changed, 67 insertions(+), 57 deletions(-)

Approvals:
  Adar Dembo: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2193: Status LogBlockManager::OpenContainerMetadataWriter(
> kinda funny that this is removed again in the next patch. was that a patch-
I found the conflict resolution challenging and wanted a quick out. But, it is 
pretty silly. Let me fix the order and republish.


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

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

[kudu-CR] spark: add support for fault tolerant scanner

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: spark: add support for fault tolerant scanner
..


Patch Set 9: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f3192025ca5d74197600480fd3d040d70b4bbc2
Gerrit-PatchSet: 9
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: No

[kudu-CR] fs manager: optimize tmp file deletion

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: fs_manager: optimize tmp file deletion
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] block manager: require file cache

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: block manager: require file cache
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6880/1/src/kudu/fs/block_manager.cc
File src/kudu/fs/block_manager.cc:

PS1, Line 48:   if (value == 0) {
: LOG(ERROR) << "Invalid max open files: cannot be 0";
: return false;
:   }
:   return true;
> you think there's any value in making sure it's at least something semi-rea
Dan asked me that when I was first committing the file cache code. Specifically 
he was wondering whether performance fell off a cliff when max_open_files was 
below a certain amount.

The short answer is that I don't know; it'd depend a lot on what kind of 
workloads were being run. I also don't think it's worth worrying about since 
the gflag is already rather obscured.

If I'm wrong and it turns out that users use this as a footgun more often than 
not, I'll run some workloads to establish a reasonable minimum and add it to 
the validator.


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

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

[kudu-CR] Add fault injection of EIOs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: Add fault injection of EIOs
..


Patch Set 2:

For now I've left the injection in env_posix.cc since it's only used within 
this class. If other files can take advantage of the functionality, I'll gladly 
move it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] log: shut down appender thread when idle

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log: shut down appender thread when idle
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

Line 256:   // Stopping is a bit tricky. We have to consider the following race:
> I looked at mt-log-test and I don't think it's geared to trigger this, beca
ok, lemme see if I can just add a variant of mt-log-test that you suggested, 
and see if it covers these paths with some "poor man's coverage"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id40a4cfcec96198b537c2f50be7ff1204faf96d2
Gerrit-PatchSet: 1
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: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2193: Status LogBlockManager::OpenContainerMetadataWriter(
kinda funny that this is removed again in the next patch. was that a 
patch-splitting gone awry or on purpose?


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

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

[kudu-CR] block manager: require file cache

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: block manager: require file cache
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6880/1/src/kudu/fs/block_manager.cc
File src/kudu/fs/block_manager.cc:

PS1, Line 48:   if (value == 0) {
: LOG(ERROR) << "Invalid max open files: cannot be 0";
: return false;
:   }
:   return true;
you think there's any value in making sure it's at least something 
semi-reasonable like 100?


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

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

[kudu-CR] [java client] removes duplicate checks at KuduScanToken

[Hao Hao (Code Review)]

Hao Hao has uploaded a new change for review.

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

Change subject: [java client] removes duplicate checks at KuduScanToken
..

[java client] removes duplicate checks at KuduScanToken

KUDU-579 added fault tolerant scanner support at java
client. However,it missed TODO for KUDU-1040 and results
in duplicate code at KuduScanToken. This patch removes it.

Change-Id: I7e036d5fe333009a74294d27c54fe846cdc404e9
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
1 file changed, 0 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7e036d5fe333009a74294d27c54fe846cdc404e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 

[kudu-CR] WIP [rpc] introduced per-RPC credentials policy

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: WIP [rpc] introduced per-RPC credentials policy
..


Patch Set 2:

(1 comment)

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

Line 41:   void set_authn_token(const AuthnToken& token);
> In some cases it might be necessary to clear the aggregated authn token, if
I thought this was created once per proxy, in which case it's set up, and then 
probably isn't used again.  Do we have cases where we clear the token?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2
Gerrit-PatchSet: 2
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] Add fault injection of EIOs

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new patch set (#2).

Change subject: Add fault injection of EIOs
..

Add fault injection of EIOs

This patch adds the functionality to inject EIOs in env_posix based on
a flag-specified ECMAScript regex determining which paths should fail
and a flag-specified double indicating how often these failures should
occur.

Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
---
M src/kudu/util/env_posix.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
3 files changed, 93 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] spark: add support for fault tolerant scanner

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: spark: add support for fault tolerant scanner
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6782/8/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
File 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala:

Line 27:   test("Test collect rows with non fault tolerant scanner") {
This file no longer needs to be changed, right?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f3192025ca5d74197600480fd3d040d70b4bbc2
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] WIP [rpc] introduced per-RPC credentials policy

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP [rpc] introduced per-RPC credentials policy
..


Patch Set 2:

(7 comments)

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

Line 351:   if (range.first != range.second) {
> Is this if guard needed?  Looks like it won't enter the loop anyway.
You are right -- this scope is redundant.


PS2, Line 361: con
> typo: a connection with a non-compliant credentials policy
thanks :)


Line 376:   // Test-only one-connection-per-RPC mode: try to re-establish 
connections.
> Now that we have this schedule for shutdown idea, could we make the 'rpc_re
It's a great idea.  Done.


Line 384:   break;  // No need to search futher; also, iterators are 
invalidated.
> This seems brittle, could you instead continue looping and overwrite it wit
Done


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

Line 135:   // Client-side connection map.
> could you add a doc note here about why it's a multimap, maybe
Done


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

Line 58:   ANY_BUT_AUTHN_TOKEN,
> Perhaps this would be a good time to introduce a name for this concept of '
SGTM.


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

Line 41:   void set_authn_token(const AuthnToken& token);
> I would have expected this to take a kudu::security::SignedTokenPB by value
In some cases it might be necessary to clear the aggregated authn token, if 
it's set.  To avoid using additional methods (like has_xxx(), reset_xxx()) I 
decided to use boost::optional .  Probably, it's worth changing it to passing 
boost::optional by value?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2
Gerrit-PatchSet: 2
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] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 772 insertions(+), 134 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 14
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] WIP [rpc] introduced per-RPC credentials policy

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP [rpc] introduced per-RPC credentials policy
..

WIP [rpc] introduced per-RPC credentials policy

This patch introduces policy for RPC authentication credentials.  The
authentication credentials policy brings some level of control over
the type of client-side credentials used when performing a remote
procedure call.

The idea behind this change is simple: sometimes the server's behavior
depends on the type of client's credentials used to authenticate the
client to the server in the context of the remote procedure call.

One example of such an RPC is MasterService::ConnectToMaster(): it's
impossible to receive an authentication token from the master if making
a call of MasterService::ConnectToMaster() method over a connection
established using authn token.  To get a new authn token in that case,
it's necessary to open a new connection to the master using types of
credentials but authn token (e.g., Kerberos credentials will work).

WIP: some stand-alone tests are needed.  In the follow-up patch
 there is an integration test to verify that this works as a part
 of authn token re-acquisition.

Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
11 files changed, 188 insertions(+), 48 deletions(-)


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

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

[kudu-CR] WIP Allow external miniclusters to use many data dirs

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP Allow external miniclusters to use many data dirs
..

WIP Allow external miniclusters to use many data dirs

In order to test different disk configurations, it is becoming
increasingly important to have end-to-end testing with nodes backed
by multiple directories.

This patch adds this functionality to the ExternalMiniCluster class,
which now supports the 'num_dirs_per_tserver' option, and the
ExternalDaemon class, which now supports a list of data dirs instead
of a single data dir.

The ExternalMiniCluster will create the ExternalDaemon with a list of
data dirs that reflects 'num_dirs_per_tserver' by using the original
data dir name as a prefix and appending an integer (e.g. '/dir_name'
with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and
'/dir_name-1').

A new test called disk-failure-itest is added that exercises this.

WIP: EIO-handling patch must be completed for further testing to make
sense.

Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
---
M src/kudu/fs/file_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk-failure-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/tablet/tablet_replica_mm_ops.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/status.h
11 files changed, 286 insertions(+), 32 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add fault injection of EIOs

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: Add fault injection of EIOs
..

Add fault injection of EIOs

This patch adds the functionality to inject EIOs in env_posix based on
a flag-specified ECMAScript regex determining which paths should fail
and a flag-specified double indicating how often these failures should
occur.

Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
---
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
4 files changed, 97 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 

[kudu-CR] WIP: threadpool: token-based sequencing

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: threadpool: token-based sequencing
..


Patch Set 1:

(1 comment)

this seems like a reasonable approach. Potential concerns for discussion:

- does this affect the performance of a normal ThreadPool that doesn't use the 
token feature? From looking at the code it seems like it would just be a few 
branches and no extra allocation, etc, but would be good to be sure of that

- not sure about using a string for a token vs having a more opaque token, such 
as ThreadPool::ObtainSequencer() returning a 'Sequencer' struct (which could 
well be just an int or somesuch)

- docs wise, would be good to document fairness and starvation-freedom 
properties. Particularly, the fact that this exhibits neither :-D Probably fine 
since I think we would typically use this feature in threadpools with no max 
thread count specified. (is it worth actually prohibiting use of tokens in a 
pool with a max thread count? or is it just "YMMV" situation?)

http://gerrit.cloudera.org:8080/#/c/6874/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:

Line 392:   result += c;
I think you'd probably want to add a random sleep at the beginning of this 
lambda, so that if they did accidentally execute in parallel, it would be more 
likely to actually misorder


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

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

[kudu-CR] KUDU-1952 Remove round-robin for block placement

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1952 Remove round-robin for block placement
..

KUDU-1952 Remove round-robin for block placement

This is the first of a multi-patch patchset to mitigate the
single-disk failure. Throughout the code, the term "DataDir" refers to
a data directory, which is often mounted on a distinct disk. Thus,
"disks" and "data directories" will be used interchangeably.

This patch adds a mapping from tablet to a set of disks and uses it to
replace the existing round-robin placement of blocks. Tablets are
mapped to a fixed number of disks (i.e. a DataDirGroup). New blocks
are placed randomly in directories within each tablet's DataDirGroup.

Tablet-to-group mappings are generated and stored as metadata upon
tablet creation, or upon tablet replacement during a tablet copy.
During group creation, disks are added to groups by randomly selecting
two available directories and selecting the one with fewer tablets on
it. This avoids pigeonholing new tablets to disks with relatively few
tablets, while still trending towards filling underloaded disks.

Groups are maintained when restarting the server, as they are flushed
with metadata, and are deleted upon tablet deletion.  When loading
tablet data from a previous version of Kudu, the tablet's superblock
will not have a DataDirGroup. One will be generated containing all
data directories, as the tablet's data may already be spread across
any number of disks.

As this patch only addresses block placement, it does not itself
mitigate single-disk failure. Given this, and given the tradeoff
between I/O and failure disk-failure tolerance, the default behavior
will be to spread tablet data across all disks.

Testing is done at the block manager level in block_manager-test and
log_block_manager-test, as well as in the new data_dirs-test.

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

Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
A 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.proto
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/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
38 files changed, 986 insertions(+), 190 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
Gerrit-PatchSet: 26
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP Don't suicide on EIO

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP Don't suicide on EIO
..

WIP Don't suicide on EIO

Rather than suiciding when reaching an EIO, this patch adds a
mechanism that triggers error-handling in the form of a callback.
This handler is attached to the lowest non-env operations that may
result in EIO.
E.g. PosixRWFile::Write() is an env operation that may result in an
EIO. LogBlockContainer::WriteData()'s call to it must be wrapped in
the new error-handling macro KUDU_RETURN_OR_HANDLE_EIO. Thus, all
direct readers/writers of files must now implement EIO-handling code
and wrap disk IO in KUDU_RETURN_OR_HANDLE_EIO.

Also included is a new tablet data state called TABLET_DATA_CORRUPTED
in which tablet data will not be deleted until explicitly requested
or during a tablet copy.

This patch is marked WIP, as the correct behavior after a disk fails
is not yet included. For now, upon failure, the block managers will
keep track of the dead disks and will mark the appropriate tablet
peers as failed. Additionally, the error handling has been moved
between layers offline, so more cleanup is being done to align with
the above description.

Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
A src/kudu/fs/error_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/status.h
22 files changed, 398 insertions(+), 51 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

By popular demand, I've attached below a long explanation of Kudu's approach
to object initialization and thread safety that came up during code review.

Because we don't use exceptions within Kudu, the initialization of an object
is typically done in two phases:
 1. Constructor, for operations that cannot fail.
 2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status
type, and needn't be thread safe because it is called right after the
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new
object and one to initialize an object with existing state. The idea is that
callers always use the constructor and then call one of the two initializer
functions, depending on their use case. WritablePBContainerFile is one such
object; Init() is for a brand new container file, and Open() is for a
container file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer
function for files with existing state, and it was an arbitrary function
(like Append()) that could be called at any time and by any thread. That
second responsibility is why it was thread safe. Now, OpenExisting() is just
the equivalent of CreateNew(), and so it makes sense for it to be just as
thread unsafe as CreateNew() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the
LBM didn't _need_ it to be thread safe. But, I think it was net less
confusing for Reopen() to be thread safe (and thus equivalent in semantics
to Append()) than for it to be thread unsafe (and thus exceptional when
compared to Append(), Sync(), etc.).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 65 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1549: compact LBM container metadata files at startup

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1549: compact LBM container metadata files at startup
..

KUDU-1549: compact LBM container metadata files at startup

Here's another patch to speed up LBM startup by reducing the amount of
metadata processed from disk. The idea is to find metadata files with lots
of "dead" blocks (i.e. CREATE + DELETE pairs) and to "compact" them by
rewriting the files without the dead blocks' records. The set of containers
to compact is determined by the ratio of live blocks to total blocks, and
there's a new gflag to configure that.

To make this work, I had to adjust the accounting in BlockCreated() yet
again. The new approach preserves next_block_offset_, but uses
fs_aligned_length() directly for byte-related stats. Without this change,
the aligned container stats (total_bytes and live_bytes_aligned) are
completely out of whack in a container whose metadata was compacted.

Like the dead container deletion patch, this one is quick and dirty in that
the new logic is unsuitable for real-time compaction. Also like that patch,
compaction in real-time would require non-trivial synchronization changes.

Testing is done in several places:
- BlockManagerStressTest: a "real" workload that'll compact some containers
  with additional inconsistencies injected by the LBMCorruptor.
- BlockManagerTest.TestMetadataOkayDespiteFailedWrites: filesystem errors
  are injected into metadata compaction.
- LogBlockManagerTest.TestCompactFullContainerMetadataAtStartup: explicit
  test for metadata compaction (and for the stat accounting fix).

Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/fs_report.cc
M src/kudu/fs/fs_report.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/env_posix.cc
8 files changed, 331 insertions(+), 44 deletions(-)


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

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

[kudu-CR] block manager: require file cache

[Adar Dembo (Code Review)]

Adar Dembo has uploaded a new change for review.

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

Change subject: block manager: require file cache
..

block manager: require file cache

The file cache has been in Kudu since 1.2 and we've yet to see any major
issues. So let's make it required; doing so lets us clean up some branches.

As for block_manager_max_open_files, I changed the meaning of the value 0
from "no limit" to "error"; this seemed net less disruptive than changing
the meaning of -1 from "40%" to "error" and 0 from "no limit" to "40%".

Change-Id: I9946e537e0b4abd66a9a90fc05df04232db68bc0
---
M src/kudu/fs/block_manager.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 53 insertions(+), 117 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9946e537e0b4abd66a9a90fc05df04232db68bc0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 

[kudu-CR] fs manager: optimize tmp file deletion

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: fs_manager: optimize tmp file deletion
..

fs_manager: optimize tmp file deletion

In a run of dense_node-itest, the bulk of CPU time[1] was spent
canonicalizing paths while cleaning up temporary files. This patch optimizes
that in two ways:
- Stop canonicalizing paths. We already canonicalize the WAL and data dir
  roots; that's good enough for admin-provided symlinks.
- Split WAL and data root cleaning, parallelizing the latter through the
  DataDirManager.

1. Though admittedly the majority of wall clock time was waiting on IO.

Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
5 files changed, 67 insertions(+), 57 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] spark: add support for fault tolerant scanner

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: spark: add support for fault tolerant scanner
..

spark: add support for fault tolerant scanner

This adds support to use fault tolerant scanner for spark job.
By default non fault tolerant scanner is used. To turn on fault
tolerant scanner, use job config: 'kudu.faultTolerantScan'.

Change-Id: I3f3192025ca5d74197600480fd3d040d70b4bbc2
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
5 files changed, 44 insertions(+), 9 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f3192025ca5d74197600480fd3d040d70b4bbc2
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

[kudu-CR] KUDU-1952 Remove round-robin for block placement

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1952 Remove round-robin for block placement
..

KUDU-1952 Remove round-robin for block placement

This is the first of a multi-patch patchset to mitigate the
single-disk failure. Throughout the code, the term "DataDir" refers to
a data directory, which is often mounted on a distinct disk. Thus,
"disks" and "data directories" will be used interchangeably.

This patch adds a mapping from tablet to a set of disks and uses it to
replace the existing round-robin placement of blocks. Tablets are
mapped to a fixed number of disks (i.e. a DataDirGroup). New blocks
are placed randomly in directories within each tablet's DataDirGroup.

Tablet-to-group mappings are generated and stored as metadata upon
tablet creation, or upon tablet replacement during a tablet copy.
During group creation, disks are added to groups by randomly selecting
two available directories and selecting the one with fewer tablets on
it. This avoids pigeonholing new tablets to disks with relatively few
tablets, while still trending towards filling underloaded disks.

Groups are maintained when restarting the server, as they are flushed
with metadata, and are deleted upon tablet deletion.  When loading
tablet data from a previous version of Kudu, the tablet's superblock
will not have a DataDirGroup. One will be generated containing all
data directories, as the tablet's data may already be spread across
any number of disks.

As this patch only addresses block placement, it does not itself
mitigate single-disk failure. Given this, and given the tradeoff
between I/O and failure disk-failure tolerance, the default behavior
will be to spread tablet data across all disks.

Testing is done at the block manager level in block_manager-test and
log_block_manager-test, as well as in the new data_dirs-test.

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

Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
A 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.proto
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/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_compaction.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
38 files changed, 986 insertions(+), 190 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
Gerrit-PatchSet: 25
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP Don't suicide on EIO

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP Don't suicide on EIO
..

WIP Don't suicide on EIO

Rather than suiciding when reaching an EIO, this patch adds a
mechanism that triggers error-handling in the form of a callback.
This handler is attached to the lowest non-env operations that may
result in EIO.
E.g. PosixRWFile::Write() is an env operation that may result in an
EIO. LogBlockContainer::WriteData()'s call to it must be wrapped in
the new error-handling macro KUDU_RETURN_OR_HANDLE_EIO. Thus, all
direct readers/writers of files must now implement EIO-handling code
and wrap disk IO in KUDU_RETURN_OR_HANDLE_EIO.

Also included is a new tablet data state called TABLET_DATA_CORRUPTED
in which tablet data will not be deleted until explicitly requested
or during a tablet copy.

This patch is marked WIP, as the correct behavior after a disk fails
is not yet included. For now, upon failure, the block managers will
keep track of the dead disks and will mark the appropriate tablet
peers as failed. Additionally, the error handling has been moved
between layers offline, so more cleanup is being done to align with
the above description.

Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
A src/kudu/fs/error_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
A src/kudu/tablet/tablet_peer_mm_ops.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
M src/kudu/util/status.h
27 files changed, 752 insertions(+), 56 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1549: compact LBM container metadata files at startup

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1549: compact LBM container metadata files at startup
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio, 
0.10,
:   "Desired ratio of live block metadata in log 
containers. If a "
:   "container's live to total block ratio dips below 
this value, "
:   "the container's metadata file will be compacted at 
startup.");
> hmm, I guess i was thinking experimental because who knows if a ratio is ev
I can't say I have a strong opinion on the tagging given how often I hem and 
haw about which tags to use, so I don't mind making it experimental if you 
think that's better.

I'll make the change to 0.5. Note that it's not a strict linear improvement due 
to the fixed cost associated with replacing a metadata file (e.g. an fsync() on 
the new file). That cost becomes higher and higher the fewer and fewer live 
blocks are in the file.

Offhand I don't know whether metadata processing is O(size) or O(record count). 
On spinning disks I would expect the cost of IO to outweigh the cost of 
deserialization and processing. That would mean O(size) is more significant 
than O(record count). Do you have any suggestions for how to figure this out?

If it helps, after reading through the protobuf encoding guide 
(https://developers.google.com/protocol-buffers/docs/encoding), I think CREATE 
records are 17-44 bytes and DELETE records are 13-22 bytes. The variety in size 
is a function of the timestamp (both), and the offset/length (CREATE).


http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

PS2, Line 295:   // 3. Containers in 'low_live_block_containers' will have 
their metadata
 :   //files compacted.
 :  
> hmm, trying to follow this suggestion. So, you're saying that since we alre
Correct: the LBM maintains a global map of LogBlocks, each of which is a live 
block. Apart from the timestamps, each LogBlock has enough information to be 
converted back into a CREATE BlockRecordPB. If I were to implement metadata 
file compaction in real time, I'd leverage this rather than rereading the 
records from disk.

Yes, the contents of vector is just live blocks, not all blocks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
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] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

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


Patch Set 32:

(9 comments)

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

Line 60: DEFINE_string(trusted_subnets,
We discussed offline about this, just going to write up what we decided:

If the user specifies the flag explicitly, then we won't add in unroutable IPs 
or local subnets.  Instead, we'll just use the address blocks specified.


PS32, Line 65: can completely disable this restriction
I think this sentence will read better as "Set it to '0.0.0.0/0' to allow 
unauthenticated connections from all remote IP addresses."


Line 76:   for (auto const& t : strings::Split(value, ",", 
strings::SkipEmpty())) {
> warning: the variable '__end' is copy-constructed from a const reference bu
It's typical to make the iteration variable a 'const auto&', which means the 
reference is to a non-mutable object.


PS32, Line 212: public
nit: publicly

Also, add a reference to the flag that controls it, like "... prohibited. See 
--trusted_subnets flag for more information."


Line 728:"from public routable IPs are 
prohibited",
Same here


Line 916:   Status s = Network::ParseCIDRStrings(FLAGS_trusted_subnets, 
_subnets);
You can probably just wrap this in a KUDU_CHECK_OK call.  It shouldn't ever 
fail since it's already been validated, right?


http://gerrit.cloudera.org:8080/#/c/6514/32/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:

Line 96: // (same as network byte order)
add period to end of sentence.


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

Line 73:   bool WithinNetwork(const Network& netaddr) const;
What do you think about defining this operator on Network instead of Sockaddr?  
It's debatable, but I think it makes sense to keep it there.


Line 76:   bool WithinNetworks(const std::vector& netaddrs) const;
I'm not sure it's worth defining this, since it's easily done with std::any_of 
on a variety of input collections.


-- 
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: 32
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 3:

I agree that having the columns be sortable would be excellent, then we could 
by default limit the table to something like 10 rows and allow the sort to 
determine the top 10 rows, and have the toggle button expand from there.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 3:

(but that can be for the future)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 3:

(2 comments)

Thanks for doing this!  Definitely a step forward in usability.

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

Line 13: 2. /tablets has a toggle-collapse detailed table of tablets.
I'm not sure this is an improvement, since there is no content under the 
tablets list.  Seems like most of the time we will be clicking expand anyway.  
On the tables page it makes sense, in order to get a view of what's under that 
tables list.


Line 29: Also I added a memory usage column to the detailed tablets
I'm concerned it may be misleading to call this 'memory usage', since it only 
accounts for the write buffers.  What do you think about calling it 'write 
buffer memory usage'?  Too jargony?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 3:

It is a bit more work- either rolling some javascript or picking and 
integrating the right jQuery plugin. Expect it to happen in the not-so-distant 
future, though.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 3: Code-Review+1

Thanks for the link to your cluster. The toggle-collapse stuff looks pretty 
good. Would like to see sorting on the columns but it's understandable if 
that's a lot more work.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1549: compact LBM container metadata files at startup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1549: compact LBM container metadata files at startup
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio, 
0.10,
:   "Desired ratio of live block metadata in log 
containers. If a "
:   "container's live to total block ratio dips below 
this value, "
:   "the container's metadata file will be compacted at 
startup.");
> I'll tag it advanced. I don't think experimental is appropriate given that 
hmm, I guess i was thinking experimental because who knows if a ratio is even 
the right metric? It's pretty internal-facing, so seems like the kind of thing 
we may never want to allow a user to tweak? (is it reasonable to have something 
be experimental forever?)

As for a better value, I was thinking something like 0.5 -- it's worth 
compacting if we think we can reduce startup time by at least 50%, perhaps?

Do we know how to map a metadata file size/block-count to a time, 
approximately? ie is the timing O(size) or O(record count)?


http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

PS2, Line 295:   // 3. Containers in 'low_live_block_containers' will have 
their metadata
 :   //files compacted.
 :  
> I went back and forth on this. I do like the idea of not making any on-disk
hmm, trying to follow this suggestion. So, you're saying that since we already 
have the list of live blocks for each container, we could just dump that out 
rather than rereading the original block *records*?

Also maybe I misread the original code. Is the vector here just 
the remaining _live_ blocks? If so, maybe it's not so bad, since it's bounded 
at 2x the normal "steady state" usage of the LBM, and we're always assuming 
that is a small fraction of overall TS heap. In the first read through I was 
thinking this woudl be _all_ records, not just te live ones.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
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] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/13/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 731:   if (creds_policy == CredentialsPolicy::ANY_BUT_AUTHN_TOKEN) {
It seems this part have some room for optimization: e.g., we can re-use 
token-re-acq rpc to piggy back RPCs with 'any_creds' policy.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 13
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] [webui] Improvements for when there's many tablets & cleanup

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [webui] Improvements for when there's many tablets & cleanup
..

[webui] Improvements for when there's many tablets & cleanup

This patch makes several improvements to the ui when there's
a lot of tablets:

1. /tablets displays a summary of the tablets' statuses.
2. /tablets has a toggle-collapse detailed table of tablets.
3. /table displays of summary of the tablets' statuses before the
tablet table.
4. /table has a toggle-collapse detailed table of tablets.
5. Detailed tablet tables use table-hover so it's easier to
use the table to look up information by, e.g. id.

All of these changes should help address KUDU-1974 and
KUDU-1959, by making it easier to see the overall health and
status of a table or tablet server's tablets.

Additionally, I found that many tables were not using the
 and  elements, which caused some bootstrap
table styles not to be working as intended. All tables where
this was a problem have been fixed.

Also I added a memory usage column to the detailed tablets
table on /tablets.

Scrrenshots: http://imgur.com/a/Mmfbe

Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webui_util.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/thread.cc
6 files changed, 130 insertions(+), 59 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: [webui] Improvements for when there's many tablets & cleanup
..


Patch Set 1:

(4 comments)

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

Line 17: 4. Detailed tablet tables use table-hover so it's easier to
> 5.
Done


http://gerrit.cloudera.org:8080/#/c/6870/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 256: TabletMetadataLock l(tablet.get(), TabletMetadataLock::READ);
> Would it be possible to combine all of this with the work in L278+ to avoid
Done


http://gerrit.cloudera.org:8080/#/c/6870/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

Line 424: " "
> I noticed we include this already in www/metrics.html. Should we remove it 
They get different headers-- metrics.html's is hard-coded.


http://gerrit.cloudera.org:8080/#/c/6870/1/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:

PS1, Line 243: replica->tablet()->mem_tracker()
> Can this actually be null?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [webui] Improvements for when there's many tablets & cleanup

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [webui] Improvements for when there's many tablets & cleanup
..

[webui] Improvements for when there's many tablets & cleanup

This patch makes several improvements to the ui when there's
a lot of tablets:

1. /tablets displays a summary of the tablets' statuses.
2. /tablets has a toggle-collapse detailed table of tablets.
3. /table displays of summary of the tablets' statuses before the
tablet table.
4. /table has a toggle-collapse detailed table of tablets.
5. Detailed tablet tables use table-hover so it's easier to
use the table to look up information by, e.g. id.

All of these changes should help address KUDU-1974 and
KUDU-1959, by making it easier to see the overall health and
status of a table or tablet server's tablets.

Additionally, I found that many tables were not using the
 and  elements, which caused some bootstrap
table styles not to be working as intended. All tables where
this was a problem have been fixed.

Also I added a memory usage column to the detailed tablets
table on /tablets.

Scrrenshots: http://imgur.com/a/Mmfbe

Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webui_util.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/thread.cc
6 files changed, 131 insertions(+), 59 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic3904a4b0fbb6446615cd46c8a6f30f81c832c53
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] WIP [rpc] introduced per-RPC credentials policy

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: WIP [rpc] introduced per-RPC credentials policy
..


Patch Set 2:

(7 comments)

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

Line 351:   if (range.first != range.second) {
Is this if guard needed?  Looks like it won't enter the loop anyway.


PS2, Line 361: con
typo: a connection with a non-compliant credentials policy


Line 376:   // Test-only one-connection-per-RPC mode: try to re-establish 
connections.
Now that we have this schedule for shutdown idea, could we make the 
'rpc_reopen_outbound_connections' flag more powerful by marking the connection 
to be shutdown here and continuing?


Line 384:   break;  // No need to search futher; also, iterators are 
invalidated.
This seems brittle, could you instead continue looping and overwrite it with 
the result of the erase call?  It's not an issue right now, but I think always 
doing a full loop-cycle will make this test-only codepath more similar to the 
real path.


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

Line 135:   // Client-side connection map.
could you add a doc note here about why it's a multimap, maybe

// Client-side connection map. Multiple connections could be open to a remote 
server if multiple credential policies are used for individual RPCs.


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

Line 58:   ANY_BUT_AUTHN_TOKEN,
Perhaps this would be a good time to introduce a name for this concept of 
'anything but authentication token'.  Perhaps 'Primary Credentials'?  Primary 
being Kerberos or cert, whereas authentication tokens would be 'Derived' or 
'Secondary' credentials?


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

Line 41:   void set_authn_token(const AuthnToken& token);
I would have expected this to take a kudu::security::SignedTokenPB by value, 
any reason to allow setting it to an empty optional?


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

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

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 765 insertions(+), 131 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 13
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-2001 Add UNDO size to tablet on-disk size

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2001 Add UNDO size to tablet on-disk size
..

KUDU-2001 Add UNDO size to tablet on-disk size

To make the on-disk size metric more accurate, this
patch makes DeltaTracker::EstimateOnDiskSize() count
UNDO deltas and adds a new method
DeltaTracker::EstimateOnDiskSizeForCompaction() that
retains the original behavior, which only counts
REDOs and is used for compaction scoring.

Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/tablet.cc
6 files changed, 63 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2001 Add UNDO size to tablet on-disk size
..


Patch Set 5:

> Huh. It occurs to me that this needs tests. :)

Done.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [flags] fixed typo in group flag validation logic

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [flags] fixed typo in group flag validation logic
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfdc25d491941674fd7b86d8ffa33ff2e046703b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) KUDU-1941: more validation for RPC auth flags

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1941: more validation for RPC auth flags
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [flags] fixed typo in group flag validation logic

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [flags] fixed typo in group flag validation logic
..


[flags] fixed typo in group flag validation logic

This is a follow-up for e7334c2e6607ac0fa778499eda2df3f4cfcd3fe3.

This patch fixes the typo in the original commit and adds test coverage
for the case of multiple group validators in the validation pipeline.

Change-Id: Idfdc25d491941674fd7b86d8ffa33ff2e046703b
Reviewed-on: http://gerrit.cloudera.org:8080/6860
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
(cherry picked from commit a0006dbaa77c276c6498dc4edc0228cae049738c)
Reviewed-on: http://gerrit.cloudera.org:8080/6864
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/util/flag_validators-test.cc
M src/kudu/util/flags.cc
2 files changed, 78 insertions(+), 5 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idfdc25d491941674fd7b86d8ffa33ff2e046703b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR](branch-1.3.x) KUDU-1941: more validation for RPC auth flags

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: KUDU-1941: more validation for RPC auth flags
..


KUDU-1941: more validation for RPC auth flags

With this patch, both master and tserver refuse to start if
authentication is 'required' but no authentication method is configured.

Prior to this patch, the inconsistency with the run-time configuration
could be detected at a later stage when a client would try to connect
to Kudu cluster.

Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51
Reviewed-on: http://gerrit.cloudera.org:8080/6851
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
(cherry picked from commit 87ddf0ae2584f2394bb26d36c01c16e6719659db)
Reviewed-on: http://gerrit.cloudera.org:8080/6862
Tested-by: Alexey Serbin 
Reviewed-by: Todd Lipcon 
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/rpc/messenger.cc
1 file changed, 11 insertions(+), 2 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Todd Lipcon: Looks good to me, but someone else must approve
  Alexey Serbin: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [doc] Remove beta upgrade reference

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [doc] Remove beta upgrade reference
..


[doc] Remove beta upgrade reference

Change-Id: Ibde3132f3bffd1ca81d249fa9401d408dd47ff21
Reviewed-on: http://gerrit.cloudera.org:8080/6858
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M docs/installation.adoc
1 file changed, 1 insertion(+), 6 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibde3132f3bffd1ca81d249fa9401d408dd47ff21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [doc] Remove beta upgrade reference

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [doc] Remove beta upgrade reference
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] WIP [rpc] introduced per-RPC credentials policy

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: WIP [rpc] introduced per-RPC credentials policy
..

WIP [rpc] introduced per-RPC credentials policy

This patch introduces policy for RPC authentication credentials.  The
authentication credentials policy brings some level of control over
the type of client-side credentials used when performing a remote
procedure call.

The idea behind this change is simple: sometimes the server's behavior
depends on the type of client's credentials used to authenticate the
client to the server in the context of the remote procedure call.

One example of such an RPC is MasterService::ConnectToMaster(): it's
impossible to receive an authentication token from the master if making
a call of MasterService::ConnectToMaster() method over a connection
established using authn token.  To get a new authn token in that case,
it's necessary to open a new connection to the master using types of
credentials but authn token (e.g., Kerberos credentials will work).

WIP: some stand-alone tests are needed.  In the follow-up patch
 there is an integration test to verify that this works as a part
 of authn token re-acquisition.

Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
11 files changed, 158 insertions(+), 40 deletions(-)


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

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