[kudu-CR] Create ConsensusMetadataManager

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7191/7/src/kudu/consensus/consensus_meta_manager.cc
File src/kudu/consensus/consensus_meta_manager.cc:

Line 44: // Ensure we don't interleave with other ops on this tablet.
> Isn't interleaving on Create() already a disallowed operation?
> Isn't interleaving on Create() already a disallowed operation?

Well... ConsensusMetadata::Create() will fail for the 2nd caller, if that's 
what you mean.

> Even with this locking, if you had interleaved with another Create or Load, 
> you'd just crash on the InsertOrDie, right?

No, I don't think so. The 2nd thread would have to wait until the ScopedCleanup 
clears op_in_progress_[tablet_id] and then it can enter the main block. If, at 
that time, the cmeta exists, it will get an error back from 
ConsensusMetadata::Create().

> And if you are concurrent with a Delete(), then you wouldn't crash but still 
> seems like invalid usage of the API.

I think this does the right thing with multiple concurrent ops, such as 2 
concurrent calls Create() and one to Delete(). However, all of that stuff 
should probably not be going on at once.

> What are the legal interleavings that actually could/should happen in 
> practice?

Good question. If we assume Create() and Delete() calls are externally mutexed 
for a given tablet then we can make it simpler. In the current Kudu code base, 
that would be a safe assumption due to the TSTabletManager "tablet state 
transition" reservation mechanism.

> Maybe something simpler is possible here, eg allowing concurrent Load() to 
> potentially do extra work and "first loader wins" in the case of a race?

I think concurrent Load() would be straightforward to simplify as long as only 
one ConsensusMetadata instance survives at the end. I guess the question is 
whether adding additional constraints on the API s.t. concurrent Create() and 
Delete() are illegal will come back to bite us later since it may be hard to 
detect when it occurs. But maybe it's worth getting rid of this additional 
complexity.


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

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

[kudu-CR] Make ConsensusMetadata thread-safe

[Mike Percy (Code Review)]

Hello David Ribeiro Alves, Todd Lipcon,

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

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

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

Change subject: Make ConsensusMetadata thread-safe
..

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 335 insertions(+), 147 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Make ConsensusMetadata thread-safe

[Mike Percy (Code Review)]

Hello David Ribeiro Alves, Todd Lipcon,

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

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

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

Change subject: Make ConsensusMetadata thread-safe
..

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 338 insertions(+), 147 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Make ConsensusMetadata thread-safe

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6958/6/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

Line 99:   RaftConfigPB committed_config() const;
> If this returns a copy of a heavy-weight PB it should be renamed to Committ
OK, I went through and removed all of the hot-path copies that I found. Also 
renamed the copying methods to InitialCaps convention to make them more 
obviously copying.


Line 106:   RaftConfigPB pending_config() const;
> same
Done


Line 114:   RaftConfigPB active_config() const;
> same
Done


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

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

[kudu-CR] [java] KUDU-2013 re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java] KUDU-2013 re-acquire authn token if expired
..


Patch Set 1:

(1 comment)

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

Line 668: TO_BE_RENEGOTIATED, // The connection negotiation has failed and 
has to be be re-negotiated.
> Yep, ideally it would be nice to have just DISCONNECTED state and recycle d
An update: after looking at how to avoid re-connecting a connection, I came 
into conclusion this might be too cumbersome.

If not allowing the TO_BE_RENEGOTIATED state and further re-connecting, it 
would require calling status errbacks for all the enqueued RPC messages after a 
new authentication token is acquired (we cannot silently drop the connection 
which is in TO_BE_RENEGOTIATED phase because there are enqueued messages).  So, 
this would lead the upper-level code to retry corresponding RPCs, one-at-a-time 
(basically, that's about finding or establishing a new connection via the 
connection cache and then creating new copies of the messages in the wire 
format to be put into the queuedMessages member of other Connection object).

Do we want to go that way?


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

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

[kudu-CR] [java] KUDU-2013 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/7250

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

Change subject: [java] KUDU-2013 re-acquire authn token if expired
..

[java] KUDU-2013 re-acquire authn token if expired

This patch introduces automatic authn token re-acquisition when the
current authn token expires.  The client automatically retries the RPC
that hits the token expiration error (the error to re-try is seen as
FATAL_INVALID_AUTHENTICATION_TOKEN sent by the server during connection
negotiation).

Added a few of tests to exercise the new retry logic for automatic
token re-acquisition in case of master-only operations, a bare minimum
workload scenario, and one special case of a connection to the master
opened with secondary credentials.

Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
A 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
A 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquireOpen.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
9 files changed, 656 insertions(+), 66 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Use dynamic bind address for internal + external mini clusters

[Mike Percy (Code Review)]

Hello Adar Dembo, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Use dynamic bind address for internal + external mini clusters
..

Use dynamic bind address for internal + external mini clusters

This technique helps avoid port conflicts. It assigns a unique loopback
IP, based on pid and per-cluster numbering, to every InternalMiniCluster
or ExternalMiniCluster component (including masters and tablet servers).

Changes:

1. Apply the "dynamic bind address" technique to internal mini clusters
   (previously it was only used for external mini clusters).
2. Apply it to masters (previously it was only used for tablet servers).

I will post a follow-up patch to remove some of the CMakeLists.txt
limitations related to port conflicts, although we may need to put some
back due to lack of macOS support for unique loopback addresses.

I had to refactor a few host-related APIs to do this in a relatively
non-hacky manner. All tests still pass (and should more often).

Additional changes:

* Move BindMode to MiniCluster base class.
* Fix a few brittle tests that started failing because of this change.

Change-Id: I35eff9fbf5ccf8822cfe061673bc36598dff56f0
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
A src/kudu/integration-tests/mini_cluster.cc
M src/kudu/integration-tests/mini_cluster.h
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
M src/kudu/master/sys_catalog-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/ts_tablet_manager-test.cc
20 files changed, 316 insertions(+), 192 deletions(-)


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

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

[kudu-CR] log block manager: do not fail block creation if truncate fails

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: do not fail block creation if truncate fails
..


Patch Set 1:

> seems reasonable. did you hit this in real life somehow?

Nah, noticed it while reviewing Hao's patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I96ce55eb1cd3a13f7cbfaf1bde7755c4d91a7dbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Rename MiniClusterBase to MiniCluster

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Rename MiniClusterBase to MiniCluster
..


Rename MiniClusterBase to MiniCluster

Change-Id: I40273d6ca7ac5fdc4cf2a6de374d374fe86cbf36
Reviewed-on: http://gerrit.cloudera.org:8080/7273
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/internal_mini_cluster.h
R src/kudu/integration-tests/mini_cluster.h
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/test_workload.h
5 files changed, 12 insertions(+), 12 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I40273d6ca7ac5fdc4cf2a6de374d374fe86cbf36
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java] KUDU-2013 re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java] KUDU-2013 re-acquire authn token if expired
..


Patch Set 2:

(1 comment)

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

Line 272:   
RpcHeader.ErrorStatusPB.RpcErrorCodePB.FATAL_INVALID_AUTHENTICATION_TOKEN)) {
> Could we get in a weird state here where this connection is actually a mast
An update -- the new can be created, but it's not outside of the 
ConnectionCache.  A new connection will be scheduled by 
tokenReacquirer.reacquire(), but it will be created by using the cache when 
executing ConnectToCluster.run().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: do not fail block creation if truncate fails

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: do not fail block creation if truncate fails
..


Patch Set 1: Code-Review+2

seems reasonable. did you hit this in real life somehow?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I96ce55eb1cd3a13f7cbfaf1bde7755c4d91a7dbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Create ConsensusMetadataManager

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 7:

(1 comment)

just looked at the manager impl for starters

http://gerrit.cloudera.org:8080/#/c/7191/7/src/kudu/consensus/consensus_meta_manager.cc
File src/kudu/consensus/consensus_meta_manager.cc:

Line 44: // Ensure we don't interleave with other ops on this tablet.
Isn't interleaving on Create() already a disallowed operation?

Even with this locking, if you had interleaved with another Create or Load, 
you'd just crash on the InsertOrDie, right?

And if you are concurrent with a Delete(), then you wouldn't crash but still 
seems like invalid usage of the API.

What are the legal interleavings that actually could/should happen in practice? 
Maybe something simpler is possible here, eg allowing concurrent Load() to 
potentially do extra work and "first loader wins" in the case of a race?


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

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

[kudu-CR] log block manager: do not fail block creation if truncate fails

[Adar Dembo (Code Review)]

Hello Anonymous Coward #314, Todd Lipcon,

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

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

to review the following change.

Change subject: log block manager: do not fail block creation if truncate fails
..

log block manager: do not fail block creation if truncate fails

RETURN_NOT_OK here means WritableBlock::Close will fail unnecessarily. I
don't _think_ that could lead to on-disk corruption (the container has to be
full, so no blocks will be added to it after this one), but it'll cause some
inconsistency in metrics.

Change-Id: I96ce55eb1cd3a13f7cbfaf1bde7755c4d91a7dbd
---
M src/kudu/fs/log_block_manager.cc
1 file changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I96ce55eb1cd3a13f7cbfaf1bde7755c4d91a7dbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Anonymous Coward #314
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Rename MiniClusterBase to MiniCluster

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Rename MiniClusterBase to MiniCluster
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40273d6ca7ac5fdc4cf2a6de374d374fe86cbf36
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 8:

(33 comments)

I reviewed everything outside the LBM, and then began reviewing the LBM 
changes. I stopped because I saw two pretty serious issues that we need to 
think about:

Container thread safety. WritableBlocks aren't supposed to be shared by 
multiple threads, which meant that containers were also single threaded as 
blocks were added to them. Now that's no longer the case, and this introduces 
some synchronization issues when accessing container state. I think they can be 
addressed with the introduction of a per-container lock (and careful thinking 
about the right order in which to acquire the container lock and the LBM lock).

Consistent container in-memory accounting when operations fail. KUDU-1793 was a 
serious issue, and to fix it, I tried to guarantee that container in-memory 
state is only updated after the "point of no return" (that is, when a block was 
guaranteed to be successfully written). This change explicitly does away with 
that due to how Finalize() works. There's no way around this, but it means we 
need to think very carefully about what happens to container in-memory state in 
the event of a failure after a block is finalized.

http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/bloomfile.h
File src/kudu/cfile/bloomfile.h:

PS8, Line 47: blocks
It's just one block, right? Why 'blocks'?


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

Line 283: block_->Finalize();
RETURN_NOT_OK.


Line 289: block_->Finalize();
RETURN_NOT_OK.


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

PS8, Line 109: blocks
Just one block.


http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:

Line 396: 
Nit: why add an empty line here and not in the WriterThread/ReaderThread 
functions, which do the same thing? Or was this a mistake?


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

Line 995:   ASSERT_OK(transaction.CommitWrites());
Before committing, how about trying to open all the blocks in block_ids and 
verifying that they can't be found?


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

Line 143: 
> yes, there is some cases the block is flushed but not finalized, you can ca
I think FLAGS_cfile_do_on_finish has mostly outlived its usefulness (it was 
used for performance experiments early on). So let's merge the "block is 
finalized" and "block is flushing" states. This means you should make two 
gflags changes:
- Change cfile_do_on_finish to something like cfile_close_block_on_finish. It'd 
be a boolean with a default value of false.
- Add something like block_manager_flush_on_finalize. It'd be a boolean with a 
default value of true.


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

PS8, Line 73: For LogWritableBlock, container
: //is released for further usage to avoid unnecessarily 
creating new containers.
Generalize this example so that it doesn't refer to implementation details.


PS8, Line 76:  For LogWritableBlock,
: //it also finishes the blocks belong to the same container 
together to
: //reduce fsync() usage.
Generalize this so that it doesn't refer to a particular implementation.


Line 91: // The block is finalized as it has been fully written.
How about: "There is some dirty data in the block and no more may be written."


Line 146:   // Finalize a fully written block.
This doesn't explain what restrictions the FINALIZED state places on the 
WritableBlock. Can I keep writing to it? Can I call FlushDataAsync() on it? Is 
the block data now durably on disk? Use the comments in 
FlushDataAsync()/Append()/Close() for inspiration.


Line 147:   virtual Status Finalize() = 0;
Nit: since this has a real effect on the state of the block, please move it to 
be just above BytesAppended(). That way the "simple accessor" functions are 
grouped together, and the "complicated" functions likewise.


Line 149:   virtual int64_t offset() const { return 0; }
I don't think this belongs in WritableBlock. It doesn't make any sense to 
anyone outside a block manager implementation, because it's not even clear what 
it's an offset into (a file? a container? something else?). Why do you need it?


Line 285: // Group a set of blocks writes together in a transaction.
The names and comments here conflate "write" and "create". Please choose one 
and use it consistently.


Line 320:   std::vector created_blocks_;
Since we're now writing C++11 

[kudu-CR] [java] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java] separating Connection
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7146/13//COMMIT_MSG
Commit Message:

Line 11: The updated TabletClient has been renamed into RpcHelper.
> Wasn't it renamed to RpcProxy?
Woops, right.  Done.


http://gerrit.cloudera.org:8080/#/c/7146/15/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 464:   try {
> Shouldn't this still be a while loop?  If it throws interrupted I think it 
We discussed this over the kudu-general Slack channel and decided go keep the 
internal while(true) loop and add also update how the interrupt flag is set on 
the thread.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java] separating Connection
..

[java] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The updated TabletClient has been renamed into RpcProxy.
Also, this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
21 files changed, 1,401 insertions(+), 1,264 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Make ConsensusMetadata thread-safe

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6958/6/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

Line 99:   RaftConfigPB committed_config() const;
If this returns a copy of a heavy-weight PB it should be renamed to 
CommittedConfig.

More broadly, I'm concerned that this may actually be a perf issue. For example 
see e14b82496da992c40af3fbf2c24a97f38b1d96cc in which I changed to using 
'cmeta_->active_config()' specifically to avoid reallocating/copying the 
RaftConfigPB. I think this will regress that change and potentially introduce 
other problematic cases.

Not sure if that's the only case of a hot-path call into this but the PBs are 
pretty heavy-weight to copy on every UpdateConsensus call. (I think it's fine 
if they get copied during voting/election/etc since those are rare)


Line 106:   RaftConfigPB pending_config() const;
same


Line 114:   RaftConfigPB active_config() const;
same


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

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

[kudu-CR] Make ConsensusMetadata thread-safe

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
..


Patch Set 6: Code-Review-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Rename MiniClusterOptions to InternalMiniClusterOptions

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Rename MiniClusterOptions to InternalMiniClusterOptions
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb1a87d916fdc0a248b5b6174306cc544333685
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Rename MiniCluster to InternalMiniCluster

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Rename MiniCluster to InternalMiniCluster
..


Rename MiniCluster to InternalMiniCluster

Since we have ExternalMiniCluster, using InternalMiniCluster as a class
name for the threaded version is consistent. This also opens up the
possibility of using MiniCluster as the base class name in a follow-up
commit.

Change-Id: I7bd326a7a46f039e18c47b8e23ee1427ccf281be
Reviewed-on: http://gerrit.cloudera.org:8080/7272
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
M src/kudu/benchmarks/tpch/tpch1.cc
M src/kudu/client/client-test.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
R src/kudu/integration-tests/internal_mini_cluster-itest-base.h
R src/kudu/integration-tests/internal_mini_cluster.cc
R src/kudu/integration-tests/internal_mini_cluster.h
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/mini_cluster_base.h
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_history_gc-itest.cc
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/kudu-tool-test.cc
29 files changed, 114 insertions(+), 121 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7bd326a7a46f039e18c47b8e23ee1427ccf281be
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] server: consolidate tablet prepare pools

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: server: consolidate tablet prepare pools
..


server: consolidate tablet prepare pools

Using the new serial token functionality available in ThreadPool, we can now
safely consolidate all per-tablet prepare pools into a single server-wide
pool. Each replica allocates a token with which to submit to the prepare
pool, attaching the existing tablet-specific metrics to it.

The prepare pool is configured with no upper bound on the number of running
threads because prepare tasks can block on row/schema locks.

Change-Id: Ic393f739cc9c1b267142b20e04a1aaa5aed97cb0
Reviewed-on: http://gerrit.cloudera.org:8080/7150
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/kserver/kserver.cc
M src/kudu/kserver/kserver.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
11 files changed, 59 insertions(+), 40 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic393f739cc9c1b267142b20e04a1aaa5aed97cb0
Gerrit-PatchSet: 7
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] server: consolidate tablet prepare pools

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: server: consolidate tablet prepare pools
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic393f739cc9c1b267142b20e04a1aaa5aed97cb0
Gerrit-PatchSet: 6
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] threadpool: per-token metrics

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: threadpool: per-token metrics
..


threadpool: per-token metrics

This patch makes thread pool metrics configurable on a token by token basis.
The idea is simple: if a pool is shared by multiple entities in a server
(i.e. one token per tablet), pool operation metrics should reflect those
entities rather than be server-wide.

Change-Id: I29e3f78d0c033ff24f675cd84e7f7d65bf19954b
Reviewed-on: http://gerrit.cloudera.org:8080/7149
Tested-by: Adar Dembo 
Reviewed-by: Todd Lipcon 
---
M src/kudu/kserver/kserver.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
5 files changed, 119 insertions(+), 75 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I29e3f78d0c033ff24f675cd84e7f7d65bf19954b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] threadpool: per-token metrics

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: threadpool: per-token metrics
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29e3f78d0c033ff24f675cd84e7f7d65bf19954b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus: consolidate Raft thread pools

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: consensus: consolidate Raft thread pools
..


consensus: consolidate Raft thread pools

This patch consolidates the two thread pools used in Raft consensus: the
observer and "Raft" (for lack of a better name) pools. The former runs tasks
for infrequent events such as term and config changes while the latter is
used for periodic events such as processing heartbeat responses.

Both per-replica pools are consolidated into a single per-server pool.
Tokens are used to ensure that tasks belonging to a single replica are
logically grouped together. One token is shared by RaftConsensus (primary)
and PeerManager (secondary); this mirrors the existing sharing behavior and
is safe because token operations are thread-safe. A second serial token is
dedicated for the observer submissions, so that its (potentially)
long-running tasks don't starve any periodic Raft events.

The non-default max_threads may come as a surprise, but David showed me how
tasks submitted to either pool may sometimes lead to an fsync (mostly via
cmeta flushes). As such, it isn't safe to use the default num_cpus upper
bound, as that may cause such IO-based tasks to block other CPU-only tasks
(or worse, periodic tasks like heartbeat response processing).

What per-replica threads are not addressed by this patch?
- Failure detection thread: a threadpool isn't the right model
  for these. Something like a hash wheel timer would be ideal.
- Prepare thread pool (max_threads=1): these could be consolidated too, but
  their metrics are per-replica, and tokens don't (yet) support that.
  Meaning, if they were consolidated now, the metrics would also consolidate
  and would be less useful as a result.

Change-Id: I8958c607d11ac2e94908670c985059bef0ff5f54
Reviewed-on: http://gerrit.cloudera.org:8080/6946
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/peer_manager.cc
M src/kudu/consensus/peer_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/kserver/kserver.cc
M src/kudu/kserver/kserver.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
19 files changed, 193 insertions(+), 123 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8958c607d11ac2e94908670c985059bef0ff5f54
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] consensus: consolidate Raft thread pools

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: consensus: consolidate Raft thread pools
..


Patch Set 15: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8958c607d11ac2e94908670c985059bef0ff5f54
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] separating Connection

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java] separating Connection
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7146/13//COMMIT_MSG
Commit Message:

Line 11: The updated TabletClient has been renamed into RpcHelper.
> Done
Wasn't it renamed to RpcProxy?


http://gerrit.cloudera.org:8080/#/c/7146/15/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 464:   try {
Shouldn't this still be a while loop?  If it throws interrupted I think it 
means it didn't fully wait for the process to finish.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] threadpool: new test for pools with no max threads

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: threadpool: new test for pools with no max_threads
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I37d0bd0a05098bcc1a81ec9f1ac33e74e98781e4
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

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


threadpool: token-based task sequencing

This patch adds task sequencing to util/threadpool. Task sequencing allows
M contexts to share a single pool with N threads while also ensuring that
the pool executes tasks belonging to each context in order. Previously this
was only achievable via "one singleton pool per context", which grossly
inflated the total number of threads and overall state.

The new logic is implemented in ThreadPoolToken. Tasks submitted via tokens
are logically grouped together. They may also be sequenced, depending on the
token's execution mode. Tokens expose Wait() variants which will block on
the task group. Tokens may also be shut down before being destroyed, which
has semantics equivalent to shutting down the pool itself (i.e. tasks may no
longer be submitted with these tokens).

Some notes:
- I evaluated two other implementations. In one, tokens had an implicit
  lifecycle that was automatically managed by the threadpool. While simpler
  for clients, the threadpool was more inefficient with more allocations and
  deallocations in each submission. In the other, token submission was built
  using regular task submission. This afforded a certain separation between
  the "core" of the threadpool and the token logic, but it complicated
  locking and tracking of queued tasks.
- I tried to keep submission (whether token-based or tokenless) fast. Just
  the change from std::list to std::deque for the main queue ought to
  improve performance of tokenless submissions. The next bottleneck is
  likely to be lock contention on the global threadpool lock.

Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70
Reviewed-on: http://gerrit.cloudera.org:8080/6874
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/util/debug-util.h
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
4 files changed, 991 insertions(+), 91 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If46dc34212027b6ea5dbc2ead7c7af8be57f2c70
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] threadpool: new test for pools with no max threads

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: threadpool: new test for pools with no max_threads
..


threadpool: new test for pools with no max_threads

This test ensures that a pool created with effectively no max_threads works
as expected. That is:
1. Tokenless tasks should trigger the creation of a new thread.
2. Serial token-based tasks can create new threads, but only up to the
   number of tokens submitted against.

I intend to use this "feature" to consolidate some Raft thread pools where a
num_cpus upper bound may be undesirable (i.e. where tasks submitted to the
pools may result in blocking IO).

Change-Id: I37d0bd0a05098bcc1a81ec9f1ac33e74e98781e4
Reviewed-on: http://gerrit.cloudera.org:8080/6945
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.h
2 files changed, 66 insertions(+), 17 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I37d0bd0a05098bcc1a81ec9f1ac33e74e98781e4
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

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

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

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


Patch Set 14: Code-Review+2

-- 
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: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] server: move apply pool into KuduServer

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: server: move apply_pool into KuduServer
..


server: move apply_pool into KuduServer

The server-wide apply_pool was instantiated in two places: SysCatalogTable
(for the master) and TsTabletManager (for the tserver). This commit moves it
into KuduServer and unifies the instantiation. Note: the apply pool
semantics have not changed.

Some interesting side effects:
1. The master will now generate apply pool metrics.
2. The apply pool is shut down a little bit later in server shutdown than it
   was before, though I don't see any issues with this.

Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed
Reviewed-on: http://gerrit.cloudera.org:8080/6984
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/kserver/kserver.cc
M src/kudu/kserver/kserver.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
8 files changed, 65 insertions(+), 59 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] server: move apply pool into KuduServer

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: server: move apply_pool into KuduServer
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1863: improve overall safety of graceful server shutdown

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1863: improve overall safety of graceful server shutdown
..


KUDU-1863: improve overall safety of graceful server shutdown

What exactly is a "safe" graceful shutdown? I define it like this:
1. Stop handling network traffic (i.e. RPCs and HTTP requests):
   a. Future incoming RPCs are responded to with an error.
   b. Future outgoing RPCs are immediately rejected (i.e. their callbacks
  run on the issuing threads with an error status).
   c. Queued but not yet handled incoming RPCs are responded to with an
  error.
   d. A "black hole" is set up for anyone who tries to respond to a deferred
  incoming RPC (i.e. an RPC that was thunked from an RPC thread and may
  be waiting for external stimuli).
   e. Outgoing RPCs awaiting a response are marked as failed and their
  callbacks are run on RPC threads with an error status.
2. Wait for any RPC/HTTP-related threads to finish. They may be processing
   an incoming RPC, processing the response from an outgoing RPC, or dealing
   with a failed RPC from step 2e.
3. Shut down RPC-using subsystems. Each one is responsible for cleaning up
   its deferred incoming RPCs. If any responses are sent, they will be black
   holed as per step 1d.
4. Shut down remaining server state.

By ordering step 3 after steps 1 and 2, we guarantee that there are no
reactor threads modifying state belonging to the subsystems that we are
trying to shut down. Unfortunately, adhering to this order is very
difficult today: we have to choose between steps 1a and 1b or destroying the
RPC subsystem altogether, which causes problems in step 3. Moreover, it's
not possible to stop handling HTTP requests without tearing down state,
which leads to failures in some subsystems.

So for now, we'll settle for a modified form of the above:
1. Stop accepting new RPCs.
2. Shut down RPC-using subsystems.
3. Destroy all RPC RPC state, waiting for outstanding RPC threads to finish.
4. Shut down the rest of the server.

This patch implements this modified form of a safe shutdown:
- During shutdown, servers first disable the acceptance of new RPCs and stop
  the webserver.
- Servers now shut down master and tserver-specific subsystems before
  generic subsystems.
- Shutting down generic subsystems first explicitly waits for reactor
  threads to finish via a new synchronous Messenger::Shutdown.

The bulk of the patch is to Messenger::Shutdown and friends. While there, I
also reduced lock acquisition to just critical sections (fixing KUDU-1863 in
the process), extended the life of tls_context_ to Messenger destruction,
and changed Shutdown to be callable with registered services.

Change-Id: Ibbda29dd471e4dc1a8cb4f472cc456ccdb1e48cc
Reviewed-on: http://gerrit.cloudera.org:8080/7183
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/master/master.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server.cc
9 files changed, 176 insertions(+), 65 deletions(-)

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



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

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

[kudu-CR] kserver: introduce KuduServer class

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: kserver: introduce KuduServer class
..


kserver: introduce KuduServer class

The kserver module (with the KuduServer class) is an abstraction that sits
between ServerBase and Master/TabletServer. The idea is for it to
encapsulate all Kudu-specific knowledge that should be shared by Masters and
Tablet Servers without polluting ServerBase, which has no Kudu-specific
knowledge and could be reused by any C++ application.

This patch introduces the new class (just lifecycle methods for now) and
wires it into Master/TabletServer. There's no new functionality.

Change-Id: I41ffd09ce46d9d744aa25343cec77b7a33b88563
Reviewed-on: http://gerrit.cloudera.org:8080/7239
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
A src/kudu/kserver/CMakeLists.txt
A src/kudu/kserver/kserver.cc
A src/kudu/kserver/kserver.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_server.h
11 files changed, 192 insertions(+), 43 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I41ffd09ce46d9d744aa25343cec77b7a33b88563
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java] separating Connection
..

[java] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The updated TabletClient has been renamed into RpcHelper.
Also, this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
21 files changed, 1,396 insertions(+), 1,252 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/15
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1929: [rpc] Allow using encrypted private keys for TLS

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-1929: [rpc] Allow using encrypted private keys for TLS
..


KUDU-1929: [rpc] Allow using encrypted private keys for TLS

* This patch adds a new flag for a "password command" for the RPC
  private key.

 * This also makes the webserver move to using the new function
   GetPasswordFromShellCommand().

 * This also consolidates certificates from security-test-util into
   security/tests/test_certs

Testing: Adds 2 tests. One to verify that RPCs work when providing
the right password for password protected private keys, and one to
verify that the Messenger does not startup if the wrong password
is provided when using a password protected private key.

Change-Id: Ifd6369581fa426ceab11e4a10441658c7da47e81
Reviewed-on: http://gerrit.cloudera.org:8080/6635
Tested-by: Kudu Jenkins
Reviewed-by: Sailesh Mukil 
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/openssl_util_bio.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
17 files changed, 340 insertions(+), 165 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved
  Kudu Jenkins: Verified



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

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

[kudu-CR] KUDU-1929: [rpc] Allow using encrypted private keys for TLS

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-1929: [rpc] Allow using encrypted private keys for TLS
..


Patch Set 9: Code-Review+2

> Build Successful
 > 
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/8758/ : SUCCESS

The issue was that ASSERT_DEATH() emits a warning if the enclosed statement 
creates multiple threads as seen here in the docs:
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md

"A warning is emitted if multiple threads are running when a death test is 
encountered."

This fails the ASSERT_DEATH() string comparison.

To mitigate this, they suggest using a "threadsafe" option which somehow 
internally mitigates this (they didn't explain how):
:testing::FLAGS_gtest_death_test_style = "threadsafe";

I've added that to the Bad password test.

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd6369581fa426ceab11e4a10441658c7da47e81
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Use dynamic bind address for internal + external mini clusters

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Use dynamic bind address for internal + external mini clusters
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35eff9fbf5ccf8822cfe061673bc36598dff56f0
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1929: [rpc] Allow using encrypted private keys for TLS

[Sailesh Mukil (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-1929: [rpc] Allow using encrypted private keys for TLS
..

KUDU-1929: [rpc] Allow using encrypted private keys for TLS

* This patch adds a new flag for a "password command" for the RPC
  private key.

 * This also makes the webserver move to using the new function
   GetPasswordFromShellCommand().

 * This also consolidates certificates from security-test-util into
   security/tests/test_certs

Testing: Adds 2 tests. One to verify that RPCs work when providing
the right password for password protected private keys, and one to
verify that the Messenger does not startup if the wrong password
is provided when using a password protected private key.

Change-Id: Ifd6369581fa426ceab11e4a10441658c7da47e81
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/openssl_util_bio.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
17 files changed, 340 insertions(+), 165 deletions(-)


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

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

[kudu-CR] [java] update dependencies and clean up some deprecate method usage

[Grant Henke (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [java] update dependencies and clean up some deprecate method 
usage
..

[java] update dependencies and clean up some deprecate method usage

Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java
M 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M 
java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableOutputFormat.java
M 
java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
18 files changed, 70 insertions(+), 75 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] mini cluster: Test infrastructure improvements

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: mini cluster: Test infrastructure improvements
..


Patch Set 4:

(1 comment)

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

PS4, Line 49: ts_map_[mts->uuid()]
A small nit which I didn't notice on the first pass.  Feel free to ignore: 

Would it make sense make ts_map_ private and to add a method to get a pointer 
to the tablet server given its UUID (something like we have in x-mini-clusters?)

Or for some reason we want ts_map_ to be exposed for access and modification by 
tests scenarios?


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

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

[kudu-CR] mini cluster: Test infrastructure improvements

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: mini cluster: Test infrastructure improvements
..

mini cluster: Test infrastructure improvements

This patch adds a couple small improvements to the MiniCluster infra.

* Add base class helpers to MiniClusterITestBase to avoid code
  duplication, including StopCluster() and ts_map_ for convenience and
  consistency with ExternalMiniClusterITestBase.
* Add ListTablets() helper function to MiniTabletServer.

Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_tablet-itest.cc
A src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
6 files changed, 82 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6bc6a04b113e59243fb788fec15b9935c3dcf069
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Create ConsensusMetadataManager

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

This patch creates an API for a class that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:

1) An instance of this class can be plumbed throughout the various
classes that deal with reading, creating, and modifying consensus
metadata so that we don't have to pass the individual cmeta instances
around.

2) It provides additional abstraction and flexibility that will make it
easier to change the underlying implementation of ConsensusMetadata in
the future as needed, perhaps if we wanted to make them log structured
and group committed across all replicas on a tablet server.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.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/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 574 insertions(+), 139 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Rename MiniCluster to InternalMiniCluster

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Rename MiniCluster to InternalMiniCluster
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7272/2/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

PS2, Line 110: MiniMaster
> honestly the true ROI of this whole rename is somewhat questionable though 
I agree with you and the rename was taking up too much time. I don't want to 
prioritize doing more right now but I'll try to go back and do the rest of it 
soonish.

Somewhat related, I think it might make sense to also add an inheritance 
hierarchy for the MiniTabletServer / MiniMaster too and get rid of the 
TServerDetails struct so we can operate on generic MiniCluster / Mini* 
interfaces with our cluster test utility code. But that is a bit further down 
the road.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bd326a7a46f039e18c47b8e23ee1427ccf281be
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] update dependencies and clean up some deprecate method usage

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java] update dependencies and clean up some deprecate method 
usage
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Ensure the shaded packages are always in the same location

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: [java] Ensure the shaded packages are always in the same 
location
..


[java] Ensure the shaded packages are always in the same location

Currently when a module depends on kudu-clients and is
shaded it is possible to get bloat in the jar due to
dependencies like guava being included in the shaded
jar twice.

This moves the shade plugin and relocation configuration
to the parent pom's pluginManagement section to ensure the
same packages are always placed in the same location.

For example this reduces the kudu-client-tools jar
from 13.4 MB to 8.5 MB.

Note: The relocation target is changed from
org.apache.kudu.clients.shaded to
org.apache.kudu.shaded since the shading is not specific to
the clients anymore.

Change-Id: I3b519ae75a62ed9bdbf37b7132a43392dd9082db
Reviewed-on: http://gerrit.cloudera.org:8080/7305
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M java/kudu-client-tools/pom.xml
M java/kudu-client/pom.xml
M java/kudu-flume-sink/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
6 files changed, 38 insertions(+), 64 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3b519ae75a62ed9bdbf37b7132a43392dd9082db
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Create ConsensusMetadataManager

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

This patch creates an API for a class that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:

1) An instance of this class can be plumbed throughout the various
classes that deal with reading, creating, and modifying consensus
metadata so that we don't have to pass the individual cmeta instances
around.

2) It provides additional abstraction and flexibility that will make it
easier to change the underlying implementation of ConsensusMetadata in
the future as needed, perhaps if we wanted to make them log structured
and group committed across all replicas on a tablet server.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.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/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 574 insertions(+), 135 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Make ConsensusMetadata thread-safe

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
..


Patch Set 6: Verified+1

overriding flaky test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Ensure the shaded packages are always in the same location

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java] Ensure the shaded packages are always in the same 
location
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b519ae75a62ed9bdbf37b7132a43392dd9082db
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] remove unused scalamock dependency

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java] remove unused scalamock dependency
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I97005bd5211abd0b119373ec265b8e9d01b385cf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] remove unused scalamock dependency

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: [java] remove unused scalamock dependency
..


[java] remove unused scalamock dependency

Change-Id: I97005bd5211abd0b119373ec265b8e9d01b385cf
Reviewed-on: http://gerrit.cloudera.org:8080/7292
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M java/kudu-spark-tools/pom.xml
1 file changed, 0 insertions(+), 6 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I97005bd5211abd0b119373ec265b8e9d01b385cf
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java] Ensure the shaded packages are always in the same location

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: [java] Ensure the shaded packages are always in the same 
location
..

[java] Ensure the shaded packages are always in the same location

Currently when a module depends on kudu-clients and is
shaded it is possible to get bloat in the jar due to
dependencies like guava being included in the shaded
jar twice.

This moves the shade plugin and relocation configuration
to the parent pom's pluginManagement section to ensure the
same packages are always placed in the same location.

For example this reduces the kudu-client-tools jar
from 13.4 MB to 8.5 MB.

Note: The relocation target is changed from
org.apache.kudu.clients.shaded to
org.apache.kudu.shaded since the shading is not specific to
the clients anymore.

Change-Id: I3b519ae75a62ed9bdbf37b7132a43392dd9082db
---
M java/kudu-client-tools/pom.xml
M java/kudu-client/pom.xml
M java/kudu-flume-sink/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
6 files changed, 38 insertions(+), 64 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b519ae75a62ed9bdbf37b7132a43392dd9082db
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 

[kudu-CR] [java] Ensure the shaded packages are always in the same location

[Grant Henke (Code Review)]

Grant Henke has abandoned this change.

Change subject: [java] Ensure the shaded packages are always in the same 
location
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I6423a22e3cc586c76e16105582d910dc37a79b49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java] Ensure the shaded packages are alway in the same location

[Grant Henke (Code Review)]

Grant Henke has abandoned this change.

Change subject: [java] Ensure the shaded packages are alway in the same location
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib830ba386313763ad96511dc19a65beb3bb24543
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java] Ensure the shaded packages are always in the same location

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: [java] Ensure the shaded packages are always in the same 
location
..

[java] Ensure the shaded packages are always in the same location

Currently when a module depends on kudu-clients and isshaded it is possible to 
get bloat in the jar due todependencies like guava being included in the 
shadedjar twice.This moves the shade plugin and relocation configurationto the 
parent pom's pluginManagement section to ensure thesame packages are always 
placed in the same location.For example this reduces the kudu-client-tools 
jarfrom 13.4 MB to 8.5 MB.Note: The relocation target is changed 
fromorg.apache.kudu.clients.shaded toorg.apache.kudu.shaded since the shading 
is not specific tothe clients anymore.Change-Id: 
Ib830ba386313763ad96511dc19a65beb3bb24543

Change-Id: I6423a22e3cc586c76e16105582d910dc37a79b49
---
M java/kudu-client-tools/pom.xml
M java/kudu-client/pom.xml
M java/kudu-flume-sink/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
6 files changed, 38 insertions(+), 64 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6423a22e3cc586c76e16105582d910dc37a79b49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 

[kudu-CR] [java] Ensure the shaded packages are alway in the same location

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: [java] Ensure the shaded packages are alway in the same location
..

[java] Ensure the shaded packages are alway in the same location

Currently when a module depends on kudu-clients and is
shaded it is possible to get bloat in the jar due to
dependencies like guava being included in the shaded
jar twice.

This moves the shade plugin and reolocation configuration
to the parent poms pluginManagement section to ensure the
same packages are always placed in the same location.

For example this reduces the kudu-client-tools jar
from 13.4 MB to 8.5 MB.

Note: The relocation target is changed from
org.apache.kudu.clients.shaded to
org.apache.kudu.shaded since the shading is not specific to
the clients anymore.

Change-Id: Ib830ba386313763ad96511dc19a65beb3bb24543
---
M java/kudu-client-tools/pom.xml
M java/kudu-client/pom.xml
M java/kudu-flume-sink/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
6 files changed, 38 insertions(+), 64 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib830ba386313763ad96511dc19a65beb3bb24543
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 

[kudu-CR] [java] update dependencies and clean up some deprecate method usage

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: [java] update dependencies and clean up some deprecate method 
usage
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7276/5/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java:

Line 463:   throw new IOException("no peer cert found");
> Sorry I was a bit unclear - this should remain an SSLPeerUnverifiedExceptio
Done


Line 582:   throw new IOException("no channel bindings provided by remote 
peer");
> Actually these could be 'SSLPeerUnverifiedException' as well, that probably
Done


http://gerrit.cloudera.org:8080/#/c/7276/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
File 
java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java:

Line 454:.add("a", a)
> align periods vertically ('chop-down' style)
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java] update dependencies and clean up some deprecate method usage

[Grant Henke (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java] update dependencies and clean up some deprecate method 
usage
..

[java] update dependencies and clean up some deprecate method usage

Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java
M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java
M 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java
M 
java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableOutputFormat.java
M 
java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/ITInputFormatJob.java
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
18 files changed, 72 insertions(+), 76 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Make ConsensusMetadata thread-safe

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
..


Patch Set 6: Code-Review+2

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

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

[kudu-CR] ConsensusMetadata::Create() should not overwrite an existing file

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: ConsensusMetadata::Create() should not overwrite an existing 
file
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bfef1ae86a7d2c162095a76bb184a9c18dc69a7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Rename MiniCluster to InternalMiniCluster

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Rename MiniCluster to InternalMiniCluster
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7272/2/src/kudu/integration-tests/internal_mini_cluster.h
File src/kudu/integration-tests/internal_mini_cluster.h:

PS2, Line 110: MiniMaster
> Adar brought this up earlier; see my earlier response. It's just low ROI gr
honestly the true ROI of this whole rename is somewhat questionable though I do 
see the consistency point. Seems to me that if we are to be consistent and went 
the extra mile to do so we should be so all over, it doesn't make a lot of 
sense have an InternalMiniCluster with a MiniMaster/MiniTabletServer that is 
not explicitly internal. I don't understand why we'd draw the line there as it 
seems like the job is unfinished (which it didn't seem before this rename). I 
don't mind doing it in another patch, but I do think we should do it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bd326a7a46f039e18c47b8e23ee1427ccf281be
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TabletReplica: Move Init() logic to Start()

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: TabletReplica: Move Init() logic to Start()
..

TabletReplica: Move Init() logic to Start()

This is a cleanup / refactor that will make it easier to implement
tombstoned voting. In this patch we merge Init() and Start() since they
aren't really logically different right now.

An unrelated change in this patch is a minor API cleanup in
TSTabletManager, where we now pass TabletReplica directly to
TSTabletManager::OpenTablet() instead of registering it in a map first
and then looking it up again later.

Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a
---
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
8 files changed, 151 insertions(+), 166 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Rename MiniClusterBase to MiniCluster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Rename MiniClusterBase to MiniCluster
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] Rename MiniCluster to InternalMiniCluster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Rename MiniCluster to InternalMiniCluster
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bd326a7a46f039e18c47b8e23ee1427ccf281be
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] KUDU-2013 re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java] KUDU-2013 re-acquire authn token if expired
..


Patch Set 1:

(21 comments)

Thank you for the review.  I'll address the major points in the next patch 
revision.  Right now I wanted to quickly respond and bring the code up-to-date 
with the updated revision of the parent patch.

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

PS1, Line 9: current
> 'the current'
Done


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

Line 272:   private RpcProxy newRpcProxy(final ServerInfo serverInfo, boolean 
usePrimaryCredentials) {
> Put this directly under the 1-arg version, and copy over the relevant javad
Done


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

Line 38: public class AuthnTokenReacquirer {
> Did you consider making this functionality part of SecurityContext?  Is the
Nope, I didn't think about making this is a part of SecurityContext.  I'll give 
it a try, thanks.


Line 48:   private final ReentrantLock lock = new ReentrantLock();
> It looks like this could be simplified to just be an Object with synchroniz
This sounds good, I will give it a try.


Line 54:   // TODO(aserbin): remove the client parameter
> Looks like this TODO may be stale
Done


Line 65:   // The token re-acquirer should be able to mark all the queued 
messages as failed if after some
> Should this be a TODO?
Right -- that was supposed to be a TODO.  Done.


Line 72:* @param connection connection to notify on the
> sentence fragment
Done


Line 142: if (e instanceof RecoverableException && retries++ < 5) {
> Could you move the increment of retries out of the if condition?  It's diff
Done


Line 174: ConnectToCluster.run(masterTable, masterAddresses, null,
> This won't reuse cached Master connections, right?  Don't we do that on the
This will reuse cached Master connection if the connection to the requested 
destination is present in the cache and established with primary credentials.


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

Line 102:   private final ClientSocketChannelFactory channelFactory;
> Doesn't look like this gets used outside the ctor?
Right -- done.


Line 272: if (m instanceof Negotiator.Failure) {
> Could we get in a weird state here where this connection is actually a mast
Yep, that's possible -- I replied on your comment on 
ConnectionCache.getConnection() method.


Line 282:   Preconditions.checkState(state == State.NEGOTIATING);
> Might be worth adding a check that this connection is not using primary cre
That's a good idea. Yes, if that happens, we should report an exception so it 
would be handled appropriately.


Line 668: TO_BE_RENEGOTIATED, // The connection negotiation has failed and 
has to be be re-negotiated.
> If I understand correctly, this patch is changing Connection so that it act
Yep, ideally it would be nice to have just DISCONNECTED state and recycle 
disconnected connections.  But then it's necessary to move the queued RPCs into 
a new connection.  Probably, that can be done at the ConnectionCache level.  
I'll take a closer look at that.


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

Line 100: //   3. A connection with primary credentials is requested 
but currently registered one
> Hmm, this comment would seem to contradict my earlier understanding about a
Probably, your confusion comes from the naming for the 'DISCONNECTED' state.  
One small detail: when connection negotiation fails, the state changes to 
TO_BE_RENEGOTIATED, not DISCONNECTED.  The DISCONNECTED state is a terminal 
state in the state diagram.

And yes, a Connection object starts re-negotiation, after getting into 
TO_BE_RENEGOTIATED state.  Such a connection will stay in the cache unless 
that's a connection to the master where we need to establish a new connection 
using primary credentials.  In the latter case, the re-negotiation happens 
anyway, but the re-negotiated connection is replaced in the cache with the 
primary-credentials-connection-to-master which is used to re-acquire authn 
token.  The queued RPCs from the re-negotiated old 
connection-to-the-same-master will be sent when the connection is re-negotiated 
using the new authn token.  In that case there will be two connections from the 
client to the same 

[kudu-CR] [java] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java] separating Connection
..

[java] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The updated TabletClient has been renamed into RpcHelper.
Also, this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
21 files changed, 1,389 insertions(+), 1,256 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java] KUDU-2013 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/7250

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

Change subject: [java] KUDU-2013 re-acquire authn token if expired
..

[java] KUDU-2013 re-acquire authn token if expired

This patch introduces automatic authn token re-acquisition when the
current authn token expires.  The client automatically retries the RPC
that hits the token expiration error (the error to re-try is seen as
FATAL_INVALID_AUTHENTICATION_TOKEN sent by the server during connection
negotiation).

Added a few of tests to exercise the new retry logic for automatic
token re-acquisition in case of master-only operations, a bare minimum
workload scenario, and one special case of a connection to the master
opened with secondary credentials.

Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A 
java/kudu-client/src/main/java/org/apache/kudu/client/AuthnTokenReacquirer.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
A 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
A 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquireOpen.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
13 files changed, 676 insertions(+), 78 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] [java] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java] separating Connection
..


Patch Set 10:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7146/13//COMMIT_MSG
Commit Message:

Line 11: The TabletClient has been renamed into RpcHelper.  Also,
> update 'RpcHelper'
Done


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

Line 151:   /** A trivial cache to keep track of already opened connections to 
Kudu servers. */
> Hmm so I don't think the new comment is adding much clarity.  I think i'd p
Done


Line 170:* @see ../src/kudu/common/common.proto
> This is relative to where you have set up the root IntelliJ project.  I'd p
Done


http://gerrit.cloudera.org:8080/#/c/7146/13/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 479: }
> omit the trailing period
Done


http://gerrit.cloudera.org:8080/#/c/7146/13/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java:

Line 51
> I think the @ needs to go inside the opening brace
Done


Line 61
> This comment can be omitted, it's obvious from context
Done


Line 73
> omit trailing periods on param docs
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes