[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..

[tests] clean-up on kudu::client::ScanToStrings

With this patch, kudu::client::ScanToStrings() returns Status.
The new signature is more convenient for localizing scan errors, if any.

Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
7 files changed, 38 insertions(+), 37 deletions(-)


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

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

[kudu-CR] Move CreateTableForTesting into registration-test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Move CreateTableForTesting into registration-test
..


Patch Set 3: Code-Review+2

LGTM

The only nit is unused master::GetTableLocationsRequestPB in 
registration-test.cc reported by the Tidy Bot.

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

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

[kudu-CR](gh-pages) WIP: Kudu Consistency Blog Post Pt1

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: WIP: Kudu Consistency Blog Post Pt1
..


Patch Set 3:

(32 comments)

Really appreciated explicit definitions of what and when things happen. I think 
the section about design decisions can be shortened a bit but will see after 
the rest of the post is in.

mostly comments about wording, some about content

http://gerrit.cloudera.org:8080/#/c/7019/3/_posts/2017-05-30-kudu-consistency-pt1.md
File _posts/2017-05-30-kudu-consistency-pt1.md:

Line 11: of how they all play together and what are our future plans.
would be nice to also include what is discussed in Part 1.


PS3, Line 17: users and requests
: are executed by many different machines,
how about, "...users, and must serve requests across numerous machines,..."?


PS3, Line 20: how Kudu implements this
"how these systems implement"


PS3, Line 29: single thread
"single-threaded"


Line 45: ## Motivation behind the consistency design decisions in Kudu:
Can we assume the reader understands what a tradeoff is? If we can, I think we 
can take out the first paragraph.

Maybe relabels this "Motivation behind some key design decisions in Kudu" since 
these aren't specifically consistency design decisions

The key points here I think are:
1. the overarching goal for Kudu is fast analytic queries
2. we make similar design choices as those made in the Spanner paper w.r.t 
writes
3. we targeted continuous inserts
4. we wanted to grants users flexibility of the consistency constraint


PS3, Line 95: semantics
"guarantees", "constraints"?


PS3, Line 101: Kudu currently lacks support for atomic multi-row mutation 
operations (i.e. mutation
 : operations to more than one row in the same or different 
tablets, planned as a future feature) so,
 : for the write path, we’ll be talking about the consistency 
semantics of single row mutations.
 : In this context we’ll discuss Kudu’s properties more from a 
key/value store standpoint. On the
 : other hand Kudu is an analytical storage engine so, for the read 
path, we’ll also discuss the
 : semantics of large (multi-row) scans. This moves the discussion 
more into the field of traditional
 : DBMSs. These ingredients make for a non-traditional discussion 
that is not exactly apples-to-apples
 : with what the reader might be familiar with, but our hope is 
that it still provides valuable, or
 : at least interesting, insight.
maybe consider leaving the write and read discussion to their respective 
sections, or move this entire highlighted portion to  the previous section (and 
leave this section as just "Relevant architectural components")


PS3, Line 113: Operation execution
"Transaction driver", "TransactionDriver"? trying to have each bullet's title 
be the component name


Line 117: replicated to follower replicas by a leader replica following the 
Raft consensus algorithm[ref].
", and thus, each tablet owns its own Consensus module."


PS3, Line 121: Timestamp assignment and propagation
maybe "Timestamp and Clock"? Or even have a separate bullet for "Clock".


PS3, Line 130: landed
"been added to"?


PS3, Line 130: A request is not considered “committed” until is has landed on
 : the WAL of a majority of replicas
Consider inverting this sentence, since we're introducing a bunch of terms here 
anyway:
"A request is considered "committed" when it has been appended to a majority of 
replicas' WALs and registered with MVCC as committed" (and then maybe put the 
definition for WAL after MVCC)


PS3, Line 132: This component serves a dual purpose storing the original client
 : requests and how they mutated each tablet’s internal state after 
being applied
how about: "This component serves two purposes: it stores the original logical 
client-requested op (i.e. inserts, updates, deletes), and the physical change 
to the tablet after the op is applied (i.e. flushes and compactions).


PS3, Line 136: Scans take a snapshots
"A scan will take a snapshot"


PS3, Line 136: they start executing and use
"it starts executing, and will use"


PS3, Line 137: unfinished operations as a tablet is scanned
"unfinished tablet operations as it progresses"


PS3, Line 139: component
remove


PS3, Line 159: 2. Timestamp assignment
Note that this is done by the leader


PS3, Line 161:  follow up posts
"a later post"


PS3, Line 161: now the reader can just think of
 : it
"a timestamp can be thought of"


PS3, Line 162: That timestamp
This


PS3, Line 163: related operations
Would "any two operations" be accurate as well? Or does "related operations" 
refer to something specific (e.g. operations on the same machine, in the same 
config, etc.)?


PS3, Line 164: Register the operation with MVCC
super nit: for consistency with previous bullets: "Registration of operation 

[kudu-CR] KUDU-2027 retry scan RPC if negotiation times out

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: KUDU-2027 retry scan RPC if negotiation times out
..

KUDU-2027 retry scan RPC if negotiation times out

This patch addresses KUDU-2027, i.e. with this patch the Kudu C++ client
retries a scan RPC with other tablet replica if the connection
negotiation with current replica timed out.

Added new integration test to cover the updated client's behavior.

This patch is a follow-up for e3c5dd18c22b9e20358f05dcd301e7736c0a3321,
since KUDU-2027 is a scan-related counterpart of KUDU-1580.

Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14
---
M src/kudu/client/scanner-internal.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
2 files changed, 93 insertions(+), 27 deletions(-)


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

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

[kudu-CR] consensus: Get rid of LockFor*() methods

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: consensus: Get rid of LockFor*() methods
..


Patch Set 10:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS8, Line 718: wers is disabled. Doing nothing.";
> this message could be updated now
Done


Line 1435: 
> seems like everywhere you take the lock, you are using AssertWaitAllowed.
I would rather do that in a follow-up patch since this patch is only intended 
to move code around without changing functionality. Right now we use a 
spinlock, not a mutex.


http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS10, Line 284: << "Replica is not in kInitialized state: "
> nit: I would drop this part since it does not make the message more actiona
Done


PS10, Line 313:   }
  : 
  :   {
  : ThreadRestrictions::AssertWaitAllowed();
  : LockGuard l(lock_);
> It seems this could be safely removed.
I would rather do that in a follow-up patch since this patch is not intended to 
change any functionality.


PS10, Line 1360: SnoozeFailureDetectorUnlocked()
> What if it returns non-OK status?  Should that case be handled somehow?
Nice catch, done.


PS10, Line 2120: UniqueLock
> why not LockGuard?
We need to unlock() below at L2178


Line 2178: lock.unlock();
We unlock here.


PS10, Line 2424:   RETURN_NOT_OK(CheckActiveLeaderUnlocked());
   :   return Status::OK();
> nit: this could be reduced to
Done


http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

PS8, Line 479: 
> this should probably also have WARN_UNUSED_RESULT
Done


http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

PS10, Line 225: using UniqueLock = std::unique_lock;
> I found only one place which uses the UniqueLock in the .cc file, and I'm n
see my reply on the other comment


PS10, Line 469: operation
> message?
I like the terminology of Operation better, since we are replicating things 
like Write operations. See OperationType in consensus.proto


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

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

[kudu-CR] consensus: Get rid of LockFor*() methods

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: consensus: Get rid of LockFor*() methods
..

consensus: Get rid of LockFor*() methods

Simplify the locking logic by removing layers of abstraction.

Also add State_Name() helper for state-related error messages.

Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 170 insertions(+), 207 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 11
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 

[kudu-CR] KUDU-2027 retry scan RPC if negotiation times out

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2027 retry scan RPC if negotiation times out
..

KUDU-2027 retry scan RPC if negotiation times out

This patch addresses KUDU-2027, i.e. with this patch the Kudu C++ client
retries a scan RPC with other tablet replica if the connection
negotiation with current replica timed out.

Added new integration test to cover the updated client's behavior.

This patch is a follow-up for e3c5dd18c22b9e20358f05dcd301e7736c0a3321,
since KUDU-2027 is a scan-related counterpart of KUDU-1580.

Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14
---
M src/kudu/client/scanner-internal.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
2 files changed, 95 insertions(+), 28 deletions(-)


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

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

[kudu-CR] consensus: Get rid of LockFor*() methods

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: consensus: Get rid of LockFor*() methods
..


Patch Set 10:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS10, Line 284: << "Replica is not in kInitialized state: "
nit: I would drop this part since it does not make the message more actionable.


PS10, Line 313:   }
  : 
  :   {
  : ThreadRestrictions::AssertWaitAllowed();
  : LockGuard l(lock_);
It seems this could be safely removed.


PS10, Line 1360: SnoozeFailureDetectorUnlocked()
What if it returns non-OK status?  Should that case be handled somehow?


PS10, Line 2120: UniqueLock
why not LockGuard?


PS10, Line 2424:   RETURN_NOT_OK(CheckActiveLeaderUnlocked());
   :   return Status::OK();
nit: this could be reduced to

  return CheckActiveLeaderUnlocked();


http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

PS10, Line 225: using UniqueLock = std::unique_lock;
I found only one place which uses the UniqueLock in the .cc file, and I'm not 
sure that's necessary.  Consider dropping this.


PS10, Line 469: operation
message?


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

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

[kudu-CR] consensus: Get rid of LockFor*() methods

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: consensus: Get rid of LockFor*() methods
..

consensus: Get rid of LockFor*() methods

Simplify the locking logic by removing layers of abstraction.

Also add State_Name() helper for state-related error messages.

Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 170 insertions(+), 207 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 12
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 

[kudu-CR] consensus: Get rid of LockFor*() methods

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: consensus: Get rid of LockFor*() methods
..


Patch Set 12: Code-Review+2

(3 comments)

LGTM.  You might be interested to get more feedback from Todd.

http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS10, Line 284:<< State_Name(st
> Done
agreed -- it's better to make small verifiable steps


PS10, Line 2120: UniqueLock
> We need to unlock() below at L2178
woops, I missed that.


http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

PS10, Line 225: using UniqueLock = std::unique_lock;
> see my reply on the other comment
Yep, thanks -- it seems I missed the unlock() part.


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

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

[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..


[tests] clean-up on kudu::client::ScanToStrings

With this patch, kudu::client::ScanToStrings() returns Status.
The new signature is more convenient for localizing scan errors, if any.

Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d
Reviewed-on: http://gerrit.cloudera.org:8080/7027
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
7 files changed, 41 insertions(+), 41 deletions(-)

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



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

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

[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily 
rewriting tablet info
..


KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet 
info

This adds some checks in SysCatalogTable so that, if the updated version
of a catalog table entry is identical to the previous version, we avoid
writing anything to the table.

This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing. For example, when the master
restarts or fails over, all of the tablet servers perform a full tablet
report. However, most of the tablets will not have changed any
information since their prior report, in which case the writes can be
safely skipped on the master.

This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each _changed_ reported tablet) but should still be a
substantial reduction. Perhaps most importantly, if a heartbeat from a
tserver times out due to long processing, the retry from the TS should
hit this code path and therefore be processed quickly.

Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Reviewed-on: http://gerrit.cloudera.org:8080/6916
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test-util.h
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 106 insertions(+), 17 deletions(-)

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



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

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

[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [consensus_queue] fix race in UpdateLagMetrics()
..


Patch Set 1:

(1 comment)

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

Line 603:   std::lock_guard l(queue_lock_);
> I think this is OK, but just to confirm... this is called from RaftConsensu
I took a look at the places where 
PeerMessageQueue::UpdateLastIndexAppendedToLeader() is called and I didn't find 
any suspicious places.  Actually, the only non-test place it's called is in 
RaftConsensus::UpdateReplica().  I also found that many methods like 
RaftConsensus::XxxUnlocked() call Queue methods which take the same lock, so I 
think it should be fine.

Otherwise, I was thinking, would use atomic instead.

I also ran consensus-related tests in many iterations via dist-test, and no 
issues were reported (at least in DEBUG build).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: [consensus_queue] fix race in UpdateLagMetrics()
..


Patch Set 1:

(1 comment)

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

Line 603:   std::lock_guard l(queue_lock_);
> I took a look at the places where PeerMessageQueue::UpdateLastIndexAppended
Right, that should be the only call in a non-test.

Hrm, I thought metrics were by default atomic, at least from a brief look in 
metrics.h. But we would still need locking here to prevent the race.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Alexey Serbin (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..

[tests] InvalidTokenDuringSeparateWorkloads test

Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads
test to cover authn token re-acquiring for both read-only and write-only
workloads.

This is to stay clear from the implementation details of the
TestWorkload class: it uses the same client object for both the read and
the write operations, while the authn token is stored as a shared piece
of data in the client object.

Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test
into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload.

Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
---
M src/kudu/integration-tests/authn_token_expire-itest.cc
1 file changed, 68 insertions(+), 3 deletions(-)


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

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

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..


Patch Set 2: Code-Review+2

carrying over Todd's +2

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

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

[kudu-CR] consensus: Add DCHECK(is locked()) in all unlocked methods

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: consensus: Add DCHECK(is_locked()) in all unlocked methods
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3364607e6778e6495e40d7ad36ea1850b022c444
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: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus: Get rid of LockFor*() methods

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: consensus: Get rid of LockFor*() methods
..


Patch Set 10:

think you missed a couple comments on the previous rev?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
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] Move CreateTableForTesting into registration-test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Move CreateTableForTesting into registration-test
..


Patch Set 4: Code-Review+2

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

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

[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [consensus_queue] fix race in UpdateLagMetrics()
..


Patch Set 1:

(1 comment)

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

Line 603:   std::lock_guard l(queue_lock_);
> Right, that should be the only call in a non-test.
Yep, the not-so-obvious thing was accessing that last_idx_appended_to_leader 
from multiple places, and that's exactly where the race happened.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Move CreateTableForTesting into registration-test

[Todd Lipcon (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Move CreateTableForTesting into registration-test
..

Move CreateTableForTesting into registration-test

This utility function was only used in one spot, so moving it out of the
header.

Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test-util.h
2 files changed, 69 insertions(+), 59 deletions(-)


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

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

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus: Add DCHECK(is locked()) in all unlocked methods

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: consensus: Add DCHECK(is_locked()) in all unlocked methods
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3364607e6778e6495e40d7ad36ea1850b022c444
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: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Move CreateTableForTesting into registration-test

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Move CreateTableForTesting into registration-test
..


Move CreateTableForTesting into registration-test

This utility function was only used in one spot, so moving it out of the
header.

Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2
Reviewed-on: http://gerrit.cloudera.org:8080/6961
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test-util.h
2 files changed, 69 insertions(+), 59 deletions(-)

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



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

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

[kudu-CR] KUDU-1914 Add positive .htpasswd test case

[Sam Okrent (Code Review)]

Hello Hao Hao, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1914 Add positive .htpasswd test case
..

KUDU-1914 Add positive .htpasswd test case

This commit adds a test to check if a proper user/password
pair allows a user to connect successfully to a Web UI with
.htpasswd support enabled.

Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be
---
M src/kudu/security/test/test_pass.cc
M src/kudu/security/test/test_pass.h
M src/kudu/server/webserver-test.cc
M src/kudu/util/curl_util.cc
4 files changed, 16 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] disk failure: make DataDirManager failure-aware

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: disk failure: make DataDirManager failure-aware
..

disk failure: make DataDirManager failure-aware

The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that correspond
to failed directories.

Tests are added to data_dirs-test to ensure that failed directories are
not used and are not returned as part of newly created DataDirGroups. If
no healthy directories exist, callers will return an IOError with posix
code ENODEV.

Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
3 files changed, 126 insertions(+), 8 deletions(-)


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

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

[kudu-CR] disk failure: coordinate IO error handling

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: disk failure: coordinate IO error handling
..

disk failure: coordinate IO error handling

I/O errors are spawned when calls to env functions fail. The response to
these errors may vary and may span many layers of abstraction.

For instance, if an EIO were triggered while writing a block, one
response might be to shut down the tablet whose operation just failed.
In order to do this without messing up the layering, a new entity, the
FsErrorManager is added to coordinate error handling.

Callbacks are registered with the FsErrorManager as responses to errors,
and callers to at-risk functions trigger these callbacks as errors are
spawned.

Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
A src/kudu/fs/error_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
13 files changed, 233 insertions(+), 26 deletions(-)


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

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

[kudu-CR] disk failure: basic test coverage for disk failure

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: disk failure: basic test coverage for disk failure
..

disk failure: basic test coverage for disk failure

This patch adds an EMC test that spawns three servers and triggers EIOs
on two of them to fail two different tablets.

With improper disk-failure-handling, this scenario alone would have been
enough to leave the server with only a single copy of data, as the two
servers with EIOs would have been shut down entirely.

With proper disk-failure handling, this scenario would be salvagable,
and data would be replicated on the remaining disks.

Change-Id: I8ff63ec71ab718866484b9f3ec7264bc72ecfe97
---
M src/kudu/integration-tests/multidir-cluster-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/tablet/tablet_replica_mm_ops.cc
3 files changed, 137 insertions(+), 11 deletions(-)


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

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

[kudu-CR] disk failure: handle EIOs on I/O to blocks

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: disk failure: handle EIOs on I/O to blocks
..

disk failure: handle EIOs on I/O to blocks

This patch adds the proper macros around operations that touch disk that
occur when writing or reading blocks.

Change-Id: Ia03bfb711a1b022d7516f4adb37fe9fb28ec949c
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/status.h
5 files changed, 45 insertions(+), 23 deletions(-)


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

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

[kudu-CR] KUDU-1914 Add positive .htpasswd test case

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1914 Add positive .htpasswd test case
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] disk failure: coordinate IO error handling

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: disk failure: coordinate IO error handling
..


Patch Set 1:

Definitely open to feedback on the observer pattern-ish design

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Alexey Serbin (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..

[tests] InvalidTokenDuringSeparateWorkloads test

Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads
test to cover authn token re-acquiring for both read-only and write-only
workloads.

This is to stay clear from the implementation details of the
TestWorkload class: it uses the same client object for both the read and
the write operations, while the authn token is stored as a shared piece
of data in the client object.

Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test
into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload.

Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
---
M src/kudu/integration-tests/authn_token_expire-itest.cc
1 file changed, 66 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] consensus: Get rid of ReplicaState class

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: consensus: Get rid of ReplicaState class
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80
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] [tests] clean-up on kudu::client::ScanToStrings

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus: Get rid of LockFor*() methods

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: consensus: Get rid of LockFor*() methods
..


Patch Set 10:

> think you missed a couple comments on the previous rev?

Ah, I missed your comments in the churn. Will take a look.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
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] [consensus queue] fix race in UpdateLagMetrics()

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [consensus_queue] fix race in UpdateLagMetrics()
..


Patch Set 1:

(1 comment)

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

Line 603:   std::lock_guard l(queue_lock_);
I think this is OK, but just to confirm... this is called from RaftConsensus 
while holding RaftConsensus's lock. Is that the correct ordering? RaftConsensus 
-> ConsensusQueue? Do we ever invert that, leading to possible deadlock? (I 
don't think so based on me reading, but curious if you also looked)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [consensus_queue] fix race in UpdateLagMetrics()
..

[consensus_queue] fix race in UpdateLagMetrics()

TSAN reports warnings on races on writing/reading the
QueueState::last_idx_appended_to_leader field:

CatalogManagerTskITest.LeadershipChangeOnTskGeneration: WARNING: 
ThreadSanitizer: data race (pid=2710)  Write of size 8 at 0x7d53e480 by 
thread T33 (mutexes: write M1863, write M1821):
#0 kudu::consensus::PeerMessageQueue::UpdateLastIndexAppendedToLeader(long) 
consensus/consensus_queue.cc:607:44
#1 
kudu::consensus::RaftConsensus::UpdateReplica(kudu::consensus::ConsensusRequestPB
 const*, kudu::consensus::ConsensusResponsePB*) 
consensus/raft_consensus.cc:1155:13
#2 
kudu::consensus::RaftConsensus::Update(kudu::consensus::ConsensusRequestPB 
const*, kudu::consensus::ConsensusResponsePB*) 
consensus/raft_consensus.cc:752:14
#3 
kudu::tserver::ConsensusServiceImpl::UpdateConsensus(kudu::consensus::ConsensusRequestPB
 const*, kudu::consensus::ConsensusResponsePB*, kudu::rpc::RpcContext*) 
tserver/tablet_service.cc:861:25
#4 
kudu::consensus::ConsensusServiceIf::ConsensusServiceIf(scoped_refptr
 const&, scoped_refptr 
const&)::$_1::operator()(google::protobuf::Message const*, 
google::protobuf::Message*, kudu::rpc::RpcContext*) const 
consensus/consensus.service.cc:100:13
... skipped ...

  Previous read of size 8 at 0x7d53e480 by thread T79 (mutexes: write 
M1822):
#0 kudu::consensus::PeerMessageQueue::UpdateLagMetrics() 
consensus/consensus_queue.cc:602:20
#1 kudu::consensus::PeerMessageQueue::UpdateMetrics() 
consensus/consensus_queue.cc:875:3
#2 
kudu::consensus::PeerMessageQueue::ResponseFromPeer(std::__1::basic_string const&, 
kudu::consensus::ConsensusResponsePB const&, bool*) 
consensus/consensus_queue.cc:828:5
... skipped ...

For more details, see
  
http://dist-test.cloudera.org:8080/diagnose?key=0784c33a-41b5-11e7-9f6b-0242ac11000f

This patch fixes the above race.  Also, it contains some extras:
  * The PeerMessageQueue::UpdateLagMetrics() method has been renamed
into PeerMessageQueue::UpdateLagMetricsUnlocked() and made private.
  * The PeerMessageQueue::UpdateMetrics() method has been renamed
into PeerMessageQueue::UpdateMetricsUnlocked().
  * Added DCHECK(queue_lock_.is_locked()) into all
PeerMessageQueue::XxxUnlocked() methods.

Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
2 files changed, 24 insertions(+), 16 deletions(-)


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

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

[kudu-CR] KUDU-1914 Add positive .htpasswd test case

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1914 Add positive .htpasswd test case
..


KUDU-1914 Add positive .htpasswd test case

This commit adds a test to check if a proper user/password
pair allows a user to connect successfully to a Web UI with
.htpasswd support enabled.

Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be
Reviewed-on: http://gerrit.cloudera.org:8080/7026
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/security/test/test_pass.cc
M src/kudu/security/test/test_pass.h
M src/kudu/server/webserver-test.cc
M src/kudu/util/curl_util.cc
4 files changed, 16 insertions(+), 4 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7027/1/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

Line 57: Status ScanToStrings(KuduScanner* scanner, std::vector* 
row_strings);
> mind adding WARN_UNUSED_RESULT here, just in case anyone is in the midst of
It's a good idea, thanks.


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

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

[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..

[tests] clean-up on kudu::client::ScanToStrings

With this patch, kudu::client::ScanToStrings() returns Status.
The new signature is more convenient for localizing scan errors, if any.

Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
7 files changed, 41 insertions(+), 41 deletions(-)


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

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

[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [tests] InvalidTokenDuringSeparateWorkloads test
..


[tests] InvalidTokenDuringSeparateWorkloads test

Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads
test to cover authn token re-acquiring for both read-only and write-only
workloads.

This is to stay clear from the implementation details of the
TestWorkload class: it uses the same client object for both the read and
the write operations, while the authn token is stored as a shared piece
of data in the client object.

Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test
into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload.

Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
Reviewed-on: http://gerrit.cloudera.org:8080/7025
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/authn_token_expire-itest.cc
1 file changed, 68 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] consensus: Get rid of ReplicaState class

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: consensus: Get rid of ReplicaState class
..


consensus: Get rid of ReplicaState class

Merges the logic in ReplicaState into the RaftConsensus class.
ReplicaState adds complexity but doesn't really serve a purpose anymore.

There are no functional changes in this patch.

Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80
Reviewed-on: http://gerrit.cloudera.org:8080/7007
Reviewed-by: Todd Lipcon 
Tested-by: Mike Percy 
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
D src/kudu/consensus/raft_consensus_state.cc
D src/kudu/consensus/raft_consensus_state.h
7 files changed, 700 insertions(+), 876 deletions(-)

Approvals:
  Mike Percy: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80
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] consensus: Get rid of ReplicaState class

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: consensus: Get rid of ReplicaState class
..


Patch Set 6: Verified+1

Overriding Spark test cleanup issue:

org.apache.kudu.client.NonRecoverableException: Table startswith already exists 
with id d53754f87fda4558bbaedc428a40755d

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80
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] [tests] clean-up on kudu::client::ScanToStrings

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [tests] clean-up on kudu::client::ScanToStrings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7027/1/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

Line 57: Status ScanToStrings(KuduScanner* scanner, std::vector* 
row_strings);
mind adding WARN_UNUSED_RESULT here, just in case anyone is in the midst of 
rebasing other older patches that might not be checking the result?


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

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