[kudu-CR] Add tablet state summary metrics

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Add tablet state summary metrics
..


Patch Set 5:

(4 comments)

Looking pretty good, just a couple nits.

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

Line 1530:   workload.StopAndJoin();
Why not stop the workload on line 1523, as soon as we have the number of rows 
inserted that we want? The fact that we leave it running here made me think we 
were trying to test some concurrency condition here, but that's not the case.


PS5, Line 1544: Sleep for > 10ms
This comment seems out of date.


http://gerrit.cloudera.org:8080/#/c/7980/5/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 115: DEFINE_int32(tablet_state_walk_min_period_ms, 1000,
What do you think about putting the metrics prototypes, callbacks, and 
registration into its own class with its own file? For example, a 
TsTabletMangerMetrics class in a ts_tablet_manager_metrics.cc file like 
TabletMetrics is structured. That could keep the bulk of the boilerplate out of 
this file (which is already large), except perhaps for the gflag and 
TSTabletManager::UpdateTabletStateSummaryAndReturnCount() function, which 
probably can't be easily factored out.


Line 1226:   MonoDelta period = 
MonoDelta::FromMilliseconds(FLAGS_tablet_state_walk_min_period_ms);
We can initialize this object before acquiring the lock


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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for ConsensusMetadataManager::LoadOrCreate().
* A unit test for ConsensusMetadataCreateMode::NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass, and an additional test was
added for unflushed cmeta instances.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Reviewed-on: http://gerrit.cloudera.org:8080/7988
Reviewed-by: Andrew Wong 
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 370 insertions(+), 67 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, but someone else must approve
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2137: protect against concurrent schema version change and 
tablet drop
..


Patch Set 1: Code-Review+2

LGTM, but perhaps you would want to hear from Dan or Todd as well.

As an additional verification step, I ran this code with the same parameters as 
the code from the main trunk which was failing ~1/500, and in 2 sequential 1K 
runs there were no failures:

  http://dist-test.cloudera.org//job?job_id=aserbin.1504843466.6787
  http://dist-test.cloudera.org//job?job_id=aserbin.1504844820.11618

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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 9: Code-Review+2

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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 9: Code-Review+1

(2 comments)

lgtm; Alexey may want to take another look considering cmeta_manager-test has 
changed a little.

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

PS8, Line 109: 
RETURN_NOT_OK_PREPEND(ConsensusMetadata::DeleteOnDiskData(fs_manager_, 
tablet_id),
 : Substitute("Unable to delete consensus 
metadata for tablet $0", tablet_id));
> That's true. I've just updated the header file comment to note that NotFoun
Ah right, gotcha


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS7, Line 1088: guarantees that the replica has never voted in a leader 
election.
> I think I agree with what you said. But when it comes to consensus, you hav
Hah, you're right. I guess "normal" for the purposes of my comment would be 
where no one is intentionally deleting and recreating cmeta files for tablets 
that have voted already.

That sort of thing generally doesn't happen, and if it never does, the 
assumption should always be true: the nonexistence of a cmeta file implies that 
the replica has not voted.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8006/1/src/kudu/fs/block_id.h
File src/kudu/fs/block_id.h:

Line 28: #include "kudu/gutil/hash/hash.h"
Remove?


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

Line 784:   FLAGS_block_manager_preflush_control = "never";
You set the flag to be 'never' to try to avoid the 'stable page writes' issue 
we talked about on slack? If so, can you add a comment here?


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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 7:

(2 comments)

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

PS7, Line 71: ConsensusMetadataCreateMode create_mode =
:   
ConsensusMetadataCreateMode::FLUSH_ON_CREATE,
> Hrm I'm specifically asking about LoadOrCreate, not Create--the first call 
aha. yes.


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS7, Line 1088: guarantees that the replica has never voted in a leader 
election.
> Makes sense; so for the "normal" use case, this assumption is not so much n
I think I agree with what you said. But when it comes to consensus, you have to 
be a real language lawyer, and I'm not sure what you mean by the "normal" use 
case.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 8:

(3 comments)

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

PS8, Line 109: 
RETURN_NOT_OK_PREPEND(ConsensusMetadata::DeleteOnDiskData(fs_manager_, 
tablet_id),
 : Substitute("Unable to delete consensus 
metadata for tablet $0", tablet_id));
> Doesn't this fail if the cmeta is in-memory only? Is that something we shou
That's true. I've just updated the header file comment to note that NotFound 
will be returned if the data isn't found on disk. It's not really an issue in 
practice, because the only caller (ts_tablet_manager.cc) considers 
s.IsNotFound() to be an acceptable success code from this method, per the 
header doc.


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

PS8, Line 62: the cache
> nit: hrmm, since "the cache" might be confused with something Kudu-specific
Done


PS8, Line 67:  otherwise create i
> nit: Maybe also note the insignificance of the parameters if a Load is expe
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for ConsensusMetadataManager::LoadOrCreate().
* A unit test for ConsensusMetadataCreateMode::NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass, and an additional test was
added for unflushed cmeta instances.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 370 insertions(+), 67 deletions(-)


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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 8:

(5 comments)

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

PS8, Line 109: 
RETURN_NOT_OK_PREPEND(ConsensusMetadata::DeleteOnDiskData(fs_manager_, 
tablet_id),
 : Substitute("Unable to delete consensus 
metadata for tablet $0", tablet_id));
Doesn't this fail if the cmeta is in-memory only? Is that something we should 
be worried about?


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

PS7, Line 71: ConsensusMetadataCreateMode create_mode =
:   
ConsensusMetadataCreateMode::FLUSH_ON_CREATE,
> That's something that is caught by the cache check, so the first call will 
Hrm I'm specifically asking about LoadOrCreate, not Create--the first call 
Creates and doesn't flush, the second loads from cache. And in the second call, 
the create_mode doesn't matter.

Not an actionable comment, just checking my understanding :)


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

PS8, Line 62: the cache
nit: hrmm, since "the cache" might be confused with something Kudu-specific 
(e.g. block cache, or other caches if we ever implement them), consider 
"in-memory"? I don't feel too strongly about this, but thought I'd point it out.


PS8, Line 67:  otherwise create i
nit: Maybe also note the insignificance of the parameters if a Load is expected?


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS7, Line 1088: guarantees that the replica has never voted in a leader 
election.
> It's critical. If it's not true, we lose our Raft safety guarantees and spl
Makes sense; so for the "normal" use case, this assumption is not so much 
necessary as it is just how things are, and if the assumption doesn't hold, we 
might end up in some bizarro Raft scenario.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 7:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7988/7//COMMIT_MSG
Commit Message:

Line 40: 
> nit: do you want to mention new tests for CreateOrLoad()?
Done


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:

PS7, Line 121:  N
> nit: should be lowercased
So I looked it up, and apparently you are right.


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

PS7, Line 175: is flushed to disk before returning
> This is no longer always true.
Thanks for the catch. I'll update this.


PS7, Line 185: object from disk.
> IIUC, Create(NO_FLUSH_ON_CREATE) followed by Load() is an error, right? Is 
It's not a problem in practice because end-users of this API always go through 
the ConsensusMetadataManager to acquire a handle to one of these cmeta objects.


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

PS7, Line 103: Load
> Would using LoadOrCreate here as well be appropriate?
Good call. Done.


PS7, Line 138:   {
> nit: this additional scope might be removed, I think.
Done


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

PS7, Line 62: Returns an error if it cannot be found.
> Kind of follows from my other question: IIUC, Create(NO_FLUSH_ON_CREATE) fo
Good catch; updated the comment.


PS7, Line 71: ConsensusMetadataCreateMode create_mode =
:   
ConsensusMetadataCreateMode::FLUSH_ON_CREATE,
> Just making sure I understand this, back-to-back calls to LoadOrCreate(NO_F
That's something that is caught by the cache check, so the first call will 
succeed but the second call will fail. It's tested in 
ConsensusMetadataManagerTest.TestCreateMultipleUnFlushedCMetas


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS7, Line 1088: guarantees that the replica has never voted in a leader 
election.
> I think I understand why this is true (voting --> flushing to disk). Does a
It's critical. If it's not true, we lose our Raft safety guarantees and 
split-brain becomes possible because the same replica can vote differently 
multiple times in the same term. In practice, if it happened that split brain 
scenario would probably be rare, but Murphy's Law and all that.

Todd and I discussed this on Slack and in person and we decided that there's 
only so much one can do... so we will essentially consider someone deleting 
critical files out from under us a type of byzantine failure, which means it's 
undefined behavior as far as Raft is concerned. It's very hard to do something 
reasonable to protect against stuff like that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for ConsensusMetadataManager::LoadOrCreate().
* A unit test for ConsensusMetadataCreateMode::NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass, and an additional test was
added for unflushed cmeta instances.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 366 insertions(+), 66 deletions(-)


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

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

[kudu-CR] [tests] fix flakes in delete table-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tests] fix flakes in delete_table-itest
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7972/3/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:

PS3, Line 1216: 
  : 
  : 
> Instead of adding the extra functions, could we do this in less by just wra
And another catch is: the server to remove should be different from the server 
just added, and the logic to find appropriate server is different for different 
operations.  So, the result code would not be that simple at all. :)

But anyway, I'm going to use ASSERT_EVENTUALLY() to have blanket-handling for 
any type of errors appear during the process.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7dda40ae1054becaf25963a6d301ecaed5a926f9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 7:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:

PS7, Line 121:  N
nit: should be lowercased


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

PS7, Line 175: is flushed to disk before returning
This is no longer always true.


PS7, Line 185: object from disk.
IIUC, Create(NO_FLUSH_ON_CREATE) followed by Load() is an error, right? Is this 
by design / is it at all important?


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

PS7, Line 103: Load
Would using LoadOrCreate here as well be appropriate?


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

PS7, Line 62: Returns an error if it cannot be found.
Kind of follows from my other question: IIUC, Create(NO_FLUSH_ON_CREATE) 
followed by Load() is NOT an error here, right? If so, is this difference in 
APIs by design? It's a subtle difference that might be worth documenting.


PS7, Line 71: ConsensusMetadataCreateMode create_mode =
:   
ConsensusMetadataCreateMode::FLUSH_ON_CREATE,
Just making sure I understand this, back-to-back calls to 
LoadOrCreate(NO_FLUSH_ON_CREATE) is not an error, right? It will just result in 
nothing on disk?


http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS7, Line 1088: guarantees that the replica has never voted in a leader 
election.
I think I understand why this is true (voting --> flushing to disk). Does 
anything hinge on this assumption being true?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tests] fix flakes in delete table-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tests] fix flakes in delete_table-itest
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7972/3/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:

PS3, Line 1216: 
  : 
  : 
> Instead of adding the extra functions, could we do this in less by just wra
I think that's a good idea.  I thought about using ASSERT_EVENTUALLY(), but I 
was concerned with not spotting some other transitional issues (like if 
AddServer/RemoveServer returning something else but IllegalState()).  But after 
you addressed my concerns for the similar situations in tablet_copy-itest, I 
think using ASSERT_EVENTUALLY() is good enough.

Let me adopt this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7dda40ae1054becaf25963a6d301ecaed5a926f9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 7: Code-Review+2

(3 comments)

> (6 comments)
 > 
 > > (7 comments)
 > >
 > > I have the following question: why do we need to introduce that
 > > additional state into ConsensusMetadata object? Why not to leave
 > > the OVERWRITE/NO_OVERWRITE parameter for its Flush() method?
 > 
 > After discussing this with you offline, I agree that we can
 > simplify the implementation and avoid this additional state. I'll
 > back out this portion of the patch.

Thank you for addressing my concerns!

As for now, LGTM, just a couple of nits.  Feel free to ignore, though.

http://gerrit.cloudera.org:8080/#/c/7988/7//COMMIT_MSG
Commit Message:

Line 40: 
nit: do you want to mention new tests for CreateOrLoad()?


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

PS7, Line 138:   {
nit: this additional scope might be removed, I think.


http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS6, Line 127: using consensus::ConsensusMetadataCreateMode;
> This one is needed :)
oh, now I see -- that's for LoadOrCreate() call; it seems I didn't scroll down 
enough to see that :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 15:

(2 comments)

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

PS13, Line 244: d to 
> Thanks.  Just FYI, here is some more fun facts about that: https://news.yco
That is a really cool post from Joshua Bloch. Anyway, we have to decide whether 
we prefer to have one fewer bit available or have 0 - 1 == INT_MAX. The former 
seems better to me. :)


http://gerrit.cloudera.org:8080/#/c/6968/15/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

PS15, Line 736: base_data_->OnDiskDataSize()
I believe this should be OnDiskDataSize()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
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-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] ref counted: fix move constructors to actually move

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: ref_counted: fix move constructors to actually move
..

ref_counted: fix move constructors to actually move

Ever since the move constructors for scoped_refptr were first implemented, they
didn't properly move. That is to say, they caused an unnecessary AddRef/Release
pair.

The issue is that they were passing through an rvalue-reference parameter 
without
properly using std::move(). See [1] for an explanation of this subtlety.

When combined with some other in-flight patches, this resulted in a substantial
improvement in LBM startup time, since a hashmap containing ref-counted elements
could relocate them without unnecessary reference count increments and 
decrements.
It's likely it will also improve performance elsewhere throughout Kudu.

This patch also pulls in an extra move constructor from the Chromium code base
which they claim helps Clang generate better warnings. I didn't verify that, but
figured I'd copy it while I was looking at the code.

[1] 
https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11

Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
---
M src/kudu/gutil/ref_counted.h
1 file changed, 12 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP: pb util: avoid repeated stat() calls reading files

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: WIP: pb_util: avoid repeated stat() calls reading files
..

WIP: pb_util: avoid repeated stat() calls reading files

This reduces the number of fstat syscalls while loading a host with 11M blocks
from 29.3M to 147K.

Change-Id: I27371800604bcb20bafae7946d3b3e84af094598
---
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
2 files changed, 29 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I27371800604bcb20bafae7946d3b3e84af094598
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] log block manager: use move semantics to fill in the block map

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: log_block_manager: use move semantics to fill in the block map
..

log_block_manager: use move semantics to fill in the block map

This avoids an extra ref-count increment/decrement while loading
the block map during startup. This substantially improves startup
time.

This shaved off another few percent from LBM startup time.

On a real host with 11M blocks:

Before:
I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block 
manager: real 14.348s user 0.000s sys 0.001s

After:
I0907 17:27:13.600186 14550 fs_manager.cc:335] Time spent opening block 
manager: real 13.401s user 0.000s sys 0.002s

According to the benchmark:

Before:
I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.570s  user 0.034s sys 0.001s
I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.550s  user 0.037s sys 0.001s
I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.560s  user 0.037s sys 0.001s
I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.509s  user 0.038s sys 0.001s
I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.555s  user 0.037s sys 0.001s
I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.561s  user 0.036s sys 0.001s
I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.534s  user 0.038s sys 0.000s
I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.548s  user 0.037s sys 0.001s
I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.559s  user 0.037s sys 0.001s
I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.529s  user 0.035s sys 0.001s

After:
I0907 17:07:40.230087  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.495s  user 0.032s sys 0.001s
I0907 17:07:41.642105  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.412s  user 0.044s sys 0.001s
I0907 17:07:43.082852  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.441s  user 0.040s sys 0.001s
I0907 17:07:44.512603  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.043s sys 0.000s
I0907 17:07:45.956848  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.444s  user 0.041s sys 0.000s
I0907 17:07:47.386735  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.037s sys 0.002s
I0907 17:07:48.787317  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.401s  user 0.041s sys 0.002s
I0907 17:07:50.244407  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.457s  user 0.038s sys 0.001s
I0907 17:07:51.682209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.438s  user 0.037s sys 0.001s
I0907 17:07:53.116209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.434s  user 0.037s sys 0.000s

Change-Id: I8fe65087585e118f91320ec39f537b5f3ad6552f
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
2 files changed, 12 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8fe65087585e118f91320ec39f537b5f3ad6552f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..

Add a simple benchmark to create 1M blocks and reopen LBM

Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
---
M src/kudu/fs/block_id.h
M src/kudu/fs/log_block_manager-test.cc
2 files changed, 27 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] block id: use a better hash function

[Todd Lipcon (Code Review)]

Todd Lipcon has abandoned this change.

Change subject: block_id: use a better hash function
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: switch from google::sparse hash map to sparsepp

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: log_block_manager: switch from google::sparse_hash_map to 
sparsepp
..

log_block_manager: switch from google::sparse_hash_map to sparsepp

sparsepp is updated for C++11 so it enables move semantics for the map
elements. Since the block map uses ref-counted values, being able to move them
is a big win. It also claims to be generally faster even aside from the
ability to support moves.

This improved startup time 7-8x on a real host with ~11M blocks:

Before:
I0907 17:23:50.748055 12507 fs_manager.cc:335] Time spent opening block 
manager: real 108.910s  user 0.000s sys 0.001s

After:
I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block 
manager: real 14.348s user 0.000s sys 0.001s

The LBM startup benchmark (1M blocks) improved less substantially but still 
noticeably:

Before:
I0907 17:16:54.899818 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.612s  user 0.035s sys 0.002s
I0907 17:16:57.498205 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.598s  user 0.039s sys 0.001s
I0907 17:17:00.100244 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.602s  user 0.042s sys 0.000s
I0907 17:17:02.686638 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.586s  user 0.042s sys 0.000s
I0907 17:17:05.284050 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.041s sys 0.001s
I0907 17:17:07.884395 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.600s  user 0.039s sys 0.001s
I0907 17:17:10.490550 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.606s  user 0.040s sys 0.001s
I0907 17:17:13.070114 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.580s  user 0.039s sys 0.000s
I0907 17:17:15.667062 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.040s sys 0.001s
I0907 17:17:18.258447 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.591s  user 0.042s sys 0.000s

After:
I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.570s  user 0.034s sys 0.001s
I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.550s  user 0.037s sys 0.001s
I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.560s  user 0.037s sys 0.001s
I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.509s  user 0.038s sys 0.001s
I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.555s  user 0.037s sys 0.001s
I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.561s  user 0.036s sys 0.001s
I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.534s  user 0.038s sys 0.000s
I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.548s  user 0.037s sys 0.001s
I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.559s  user 0.037s sys 0.001s
I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.529s  user 0.035s sys 0.001s

Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
---
M src/kudu/fs/log_block_manager.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
5 files changed, 35 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] Avoid a few allocations while reading PBC files

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: Avoid a few allocations while reading PBC files
..

Avoid a few allocations while reading PBC files

This reduces extra short-lived allocations while reading PBC files:
- pb_util now uses faststring buffers instead of heap-allocated arrays.
  For short reads (<32 bytes) these are stack-allocated. Since PBC files
  use length prefixes for records, this avoids a heap allocation per record.

- env_posix used to use a temporary vector for every read. Now
  it uses C-style pointer-and-length for the array to avoid the allocation.

This sped up startup on a real host with 11M blocks a few more percent:

Before:
I0907 17:27:13.600186 14550 fs_manager.cc:335] Time spent opening block 
manager: real 13.401s user 0.000s sys 0.002s

After:
I0907 17:43:46.364558 21480 fs_manager.cc:335] Time spent opening block 
manager: real 12.377s user 0.000s sys 0.001s

This also sped up the LBM startup time benchmark a little bit:

Before:
I0907 17:07:40.230087  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.495s  user 0.032s sys 0.001s
I0907 17:07:41.642105  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.412s  user 0.044s sys 0.001s
I0907 17:07:43.082852  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.441s  user 0.040s sys 0.001s
I0907 17:07:44.512603  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.043s sys 0.000s
I0907 17:07:45.956848  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.444s  user 0.041s sys 0.000s
I0907 17:07:47.386735  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.037s sys 0.002s
I0907 17:07:48.787317  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.401s  user 0.041s sys 0.002s
I0907 17:07:50.244407  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.457s  user 0.038s sys 0.001s
I0907 17:07:51.682209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.438s  user 0.037s sys 0.001s
I0907 17:07:53.116209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.434s  user 0.037s sys 0.000s

After:
I0907 17:40:07.984983 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.429s  user 0.032s sys 0.001s
I0907 17:40:09.359199 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.374s  user 0.037s sys 0.000s
I0907 17:40:10.767251 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.408s  user 0.038s sys 0.001s
I0907 17:40:12.160768 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.394s  user 0.037s sys 0.001s
I0907 17:40:13.539149 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.378s  user 0.035s sys 0.001s
I0907 17:40:14.932631 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.393s  user 0.037s sys 0.000s
I0907 17:40:16.316269 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.384s  user 0.037s sys 0.000s
I0907 17:40:17.714495 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.398s  user 0.038s sys 0.000s
I0907 17:40:19.077134 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.363s  user 0.036s sys 0.001s
I0907 17:40:20.487521 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.410s  user 0.037s sys 0.001s

Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f
---
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
2 files changed, 17 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] [tests] fix flakes in delete table-itest

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: [tests] fix flakes in delete_table-itest
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7972/3/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:

PS3, Line 1216: 
  : 
  : 
Instead of adding the extra functions, could we do this in less by just 
wrapping the logic of finding the leader and changing the config in an 
ASSERT_EVENTUALLY()? i.e.:

  // Loop in case the leader changes between adding and removing a replica.
  for (int i : {0, 1}) {
ASSERT_EVENTUALLY([&] {
  // Find leader.
  TServerDetails* leader = nullptr;
  ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, ));
  ASSERT_OK(WaitForOpFromCurrentTerm(leader, tablet_id, COMMITTED_OPID, 
kTimeout));

  // Do the config changes and wait for the master to delete the old node.
  if (i == 0) {
ASSERT_OK(AddServer(leader, tablet_id, to_add, RaftPeerPB::VOTER, 
boost::none, kTimeout));
  } else {
ASSERT_OK(RemoveServer(leader, tablet_id, to_remove, boost::none, 
kTimeout));
  }
});
  }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7dda40ae1054becaf25963a6d301ecaed5a926f9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] web ui: fix "percentage consumed" calculation

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: web ui: fix "percentage consumed" calculation
..


web ui: fix "percentage consumed" calculation

There was a misplaced cast, so the division of consumption by limit was
using an integer rather than floating point calculation. This results in
the "percentage consumed" always showing 0.

Change-Id: I07d5b7d5f44548120a9b31bfef43e23051e27d8e
Reviewed-on: http://gerrit.cloudera.org:8080/7987
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/server/default-path-handlers.cc
1 file changed, 1 insertion(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I07d5b7d5f44548120a9b31bfef43e23051e27d8e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2137: protect against concurrent schema version change and 
tablet drop
..


Patch Set 1:

> Just FYI: to me, the test reliably fails with 1/512 ratio if built
 > in debug mode:
 > 
 > http://dist-test.cloudera.org//job?job_id=aserbin.1504831083.9508
 > http://dist-test.cloudera.org//job?job_id=aserbin.1504831720.12459

I ran that with --stress_cpu_threads=8

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

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

[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2137: protect against concurrent schema version change and 
tablet drop
..


Patch Set 1:

Just FYI: to me, the test reliably fails with 1/512 ratio if built in debug 
mode:

http://dist-test.cloudera.org//job?job_id=aserbin.1504831083.9508
http://dist-test.cloudera.org//job?job_id=aserbin.1504831720.12459

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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass, and an additional test was
addedadded for unflushed cmeta instances.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 352 insertions(+), 62 deletions(-)


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

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

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 6:

(6 comments)

> (7 comments)
 > 
 > I have the following question: why do we need to introduce that
 > additional state into ConsensusMetadata object? Why not to leave
 > the OVERWRITE/NO_OVERWRITE parameter for its Flush() method?

After discussing this with you offline, I agree that we can simplify the 
implementation and avoid this additional state. I'll back out this portion of 
the patch.

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

PS6, Line 334: /*overwrite_first_flush=*/ false
> Why not just to pass the NO_OVERWRITE flag to the Flush() method as it was 
The semantics I want to maintain are that we don't overwrite on the first 
Flush() call after create, but we overwrite at any other time.


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

Line 96: return Create(tablet_id, config, initial_term, create_mode);
> Does it make sense to pass the cmeta_out here as well?
Fixed, added a unit test too.


http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:

PS6, Line 86: using consensus::ConsensusMetadataCreateMode;
> nit: is it needed?
Done


http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/tablet_copy_source_session-test.cc
File src/kudu/tserver/tablet_copy_source_session-test.cc:

PS6, Line 91: using consensus::ConsensusMetadataCreateMode;
> nit: is it needed?
Done


http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS6, Line 127: using consensus::ConsensusMetadataCreateMode;
> nit: is it needed?
This one is needed :)


PS6, Line 1093: scoped_refptr cmeta;
> nit: it seems the 'cmeta' is not used in this scope; drop it, maybe?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


KUDU-2134: Defer block transaction commit to the end of a tablet copy

KUDU-2131 uncovered a regression that causes a tablet copy session
to expire reliably if the copied tablet is large and the tserver
already has a high number of containers. Even though the issue is
mitigated by LIFO container selection, we can completely avoid it by
deferring the block transaction commit to the end of tablet copy
session. There are mainly two reasons we may still want to do so:
 1) If DownloadWALs fails there's no reason to pay the fsync() price
and commit all those blocks.
 2) While DownloadWALs is running the kernel has more time to eagerly
flush the blocks, so the fsync()s at the end could be cheaper.

This patch moves the block transaction commit out from FetchAll() and
commits all downloaded blocks before replacing the superblock.

Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Reviewed-on: http://gerrit.cloudera.org:8080/7966
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
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
3 files changed, 40 insertions(+), 36 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Mike Percy: Looks good to me, approved
  Adar Dembo: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] block id: use a better hash function

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: block_id: use a better hash function
..


Patch Set 1:

I tried making a unit test and weirdly in my unit test environment, the new 
hash was actually worse than the old one.

I also tried reverting this but leaving my other optimizations, and it turns 
out in the presence of the other fixes around refcounting, etc, and using 
sparsepp instead of sparsehash, this doesn't make a difference. So, I'll 
abandon for now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Reviewed-on: http://gerrit.cloudera.org:8080/7992
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Hello Mike Percy, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 5:

(3 comments)

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

PS4, Line 182: AS
> nit: extra indentation
Done


http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS4, Line 2488: Replic
> Replica?
Done


PS4, Line 2492: fig index is $1)", 
> how about: current committed config index is $1
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4: Code-Review+1

(3 comments)

looks good, just a couple nits

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

PS4, Line 182:   
nit: extra indentation


http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS4, Line 2488: Tablet
Replica?


PS4, Line 2492: current index is $1
how about: current committed config index is $1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest
..


Rename tombstoned_voting-itest to tombstoned_voting-imc-itest

tombstoned_voting-itest uses an InternalMiniCluster, and it would be
useful to have tombstoned voting tests that use an ExternalMiniCluster.
Rename the existing IMC-based test to make room for an EMC-based test.

Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Reviewed-on: http://gerrit.cloudera.org:8080/7991
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/tombstoned_voting-imc-itest.cc
2 files changed, 10 insertions(+), 10 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Fix flakiness of ts_tablet_manager_itest 
TestFailedTabletsAreReplaced
..


Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced

TestFailedTabletsAreReplaced manually fails the replica after only
verifying that the tablet exists, with no regard for its state. This can
cause the replica's bootstrap process to fail a check:
F0907 00:05:46.153576  2697 tablet_replica.cc:173] Check failed: BOOTSTRAPPING 
== state_ (0 vs. 2)

This is a test-only race where the replica successfully goes through the
bootstrap process, the tablet is failed in test, and
TabletReplica::Start() is called on the replica, which requires its
state to be BOOTSTRAPPING. This is not an issue seen in production, as
bootstrapping is normally only run if the replica is not failed, but it
did result in 6/1000 failures when run in release mode with
--stress_cpu_thres=32.

To fix this, the replica is failed only after it is verified to be
running. In doing so, the number of failures went from 6/1000 to 0/1000.

Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e
Reviewed-on: http://gerrit.cloudera.org:8080/7993
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Hao Hao 
Reviewed-by: Mike Percy 
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
1 file changed, 16 insertions(+), 7 deletions(-)

Approvals:
  Hao Hao: Looks good to me, but someone else must approve
  Mike Percy: Looks good to me, approved
  Adar Dembo: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Fix flakiness of ts_tablet_manager_itest 
TestFailedTabletsAreReplaced
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Simplify the interface controlling whether a Flush() is allowed to
  overwrite (clobber) an existing file by encapsulating that logic in
  the implementation, instead of the public interface to
  ConsensusMetadata. When a cmeta is instantiated via
  ConsensusMetadata::Create(), the next Flush() is not allowed to
  overwrite an existing file. In every other case, Flush() is allowed to
  overwrite an existing file.
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 345 insertions(+), 68 deletions(-)


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

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

[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest
..

Rename tombstoned_voting-itest to tombstoned_voting-imc-itest

tombstoned_voting-itest uses an InternalMiniCluster, and it would be
useful to have tombstoned voting tests that use an ExternalMiniCluster.
Rename the existing IMC-based test to make room for an EMC-based test.

Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/tombstoned_voting-imc-itest.cc
2 files changed, 10 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-test

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Rename tombstoned_voting-itest to tombstoned_voting-test
..


Patch Set 1:

(2 comments)

> tombstoned_voting-itest.cc --> tombstoned_voting-imc-itest.cc ?

Done

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

PS1, Line 11: prefix
> suffix?
Done


PS1, Line 12: prefix
> suffix?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


[iwyu] kudu-specific mappings for std::tr1 entities

Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends
from TR1 (those are used in the Kudu C++ client API).

Added --max_line_length=256 IWYU option for better reporting
on the reason for header files inclusion.  If keeping the
default setting of 80 characters, IWYU sometimes omits the information
which symbols require particular header file to be included.

Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Reviewed-on: http://gerrit.cloudera.org:8080/7998
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M .gitignore
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/kudu.imp
4 files changed, 56 insertions(+), 18 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Simplify the interface controlling whether a Flush() is allowed to
  overwrite (clobber) an existing file by encapsulating that logic in
  the implementation, instead of the public interface to
  ConsensusMetadata. When a cmeta is instantiated via
  ConsensusMetadata::Create(), the next Flush() is not allowed to
  overwrite an existing file. In every other case, Flush() is allowed to
  overwrite an existing file.
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 345 insertions(+), 68 deletions(-)


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

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

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 5:

(1 comment)

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

Line 180:   ASSERT_OK(client_->transaction_.CommitCreatedBlocks());
> Finish() has some other logic which changes the state of tablet copy. I was
yep


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2124. Don't hold session lock while initializing a 
TabletCopySession
..


Patch Set 3:

(8 comments)

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

Line 1643: Status s = itest::BeginTabletCopySession(ts, tablet_id, 
"dummy-uuid", kTimeout, );
Since the response isn't actually used, consider dropping it from 
BeginTabletCopySession. It can always be added later.


Line 1674: ASSERT_LT(lat, kInjectedLatency);
This seems risky; a variable amount of latency could be added to a failure from 
load and scheduling.


http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/tserver/tablet_copy_service-test.cc
File src/kudu/tserver/tablet_copy_service-test.cc:

Line 265: }
If you expect some contention for a fixed amount of time, it might be useful to 
add a Sleep here rather than making it a tight loop to make the test is a 
little more CPU friendly.


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

PS3, Line 185: SessionEntry* session_entry = FindOrNull(sessions_, 
session_id);
 : // Ensure that another interleaved thread has not already 
removed the failed session.
 : if (session_entry && session == session_entry->session) {
 :   sessions_.erase(session_id);
 : }
Can you add to the comment to explain the identity check? I imagine it's to 
protect against another thread not only removing this session but also 
replacing it with another one.


Line 267:   // The corresponding session is already in the process of 
initializing.
Can you use RPC_RETURN_NOT_OK here?


http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/tserver/tablet_copy_source_session.cc
File src/kudu/tserver/tablet_copy_source_session.cc:

Line 73: TAG_FLAG(tablet_copy_session_inject_latency_on_init_ms, unsafe);
Make it hidden.


Line 74: TAG_FLAG(tablet_copy_session_inject_latency_on_init_ms, runtime);
I don't see the new test changing the value of this flag at runtime; can this 
be omitted?


PS3, Line 128: TRACE("Injecting $0ms of latency due to 
--tablet_copy_session_inject_latency_on_init_ms",
 :   FLAGS_tablet_copy_session_inject_latency_on_init_ms);
Is this useful to the tests that usei t?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer
of abstraction at Block Manager to coalesce blocks deletions in a
logical operation, e.g. compaction.

By coalescing blocks deletions, the number of hole punching in LBM
will be reduced from one per block to one per log block container.
This should overall optimize the performance of operation that
involves batch deletions, such as compaction and hole repunching.

This patch is the first part of a serial. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change
any deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tserver/tablet_copy_client.h
19 files changed, 157 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Hao Hao (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..

KUDU-2134: Defer block transaction commit to the end of a tablet copy

KUDU-2131 uncovered a regression that causes a tablet copy session
to expire reliably if the copied tablet is large and the tserver
already has a high number of containers. Even though the issue is
mitigated by LIFO container selection, we can completely avoid it by
deferring the block transaction commit to the end of tablet copy
session. There are mainly two reasons we may still want to do so:
 1) If DownloadWALs fails there's no reason to pay the fsync() price
and commit all those blocks.
 2) While DownloadWALs is running the kernel has more time to eagerly
flush the blocks, so the fsync()s at the end could be cheaper.

This patch moves the block transaction commit out from FetchAll() and
commits all downloaded blocks before replacing the superblock.

Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
---
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
3 files changed, 40 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055[part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 4:

The failed test TestCopyFromCrashedSource is a known flaky test (KUDU-2109).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Simplify the interface controlling whether a Flush() is allowed to
  overwrite (clobber) an existing file by encapsulating that logic in
  the implementation, instead of the public interface to
  ConsensusMetadata. When a cmeta is instantiated via
  ConsensusMetadata::Create(), the next Flush() is not allowed to
  overwrite an existing file. In every other case, Flush() is allowed to
  overwrite an existing file.
* Made some additional method arguments optional, for convenience.

The following tests have been added:

* A unit test for NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 345 insertions(+), 65 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:

PS3, Line 339: // Create() should not clobber.
> I think the placement of this comment might be a bit confusing. Should this
removed this comment


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager-stress-test.cc
File src/kudu/consensus/consensus_meta_manager-stress-test.cc:

PS3, Line 18: #include 
> nit: prefer using 
Done


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

PS3, Line 97:   if (s.ok()) 
> Did you consider narrowing the permissible errors here? e.g. if we hit an I
Thanks for the catch. That's what I meant to do. Fixed.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.h
File src/kudu/consensus/consensus_meta_manager.h:

PS3, Line 59: ConsensusMetadataCreateMode create_mode
> What if this parameter was set to ConsensusMetadataCreateMode::FLUSH_ON_CRE
Done


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:

PS3, Line 191: scoped_refptr cmeta;
> nit: drop this and leave the last parameter of the Create() call below by d
Done


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

PS3, Line 18: #include 
> nit: consider replacing with  and adding that into the list of the
Done


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:

PS3, Line 260: scoped_refptr cmeta;
> nit: consider dropping this since it's not used in the scope and leaving th
Done


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_bootstrap-test.cc
File src/kudu/tablet/tablet_bootstrap-test.cc:

PS3, Line 146: scoped_refptr cmeta;
> Nit: it seems this is not used, consider dropping it and leaving the last p
Done


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:

PS3, Line 141: scoped_refptr cmeta;
> Nit: consider dropping this since it's not used, leaving the last parameter
Done


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

PS3, Line 601: scoped_refptr cmeta;
> Nit: cmeta is not used in this scope, right?  Consider dropping it.
Done


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_source_session-test.cc
File src/kudu/tserver/tablet_copy_source_session-test.cc:

PS3, Line 157: scoped_refptr cmeta;
> nit: consider dropping cmeta -- it's not used in this scope elsewhere but p
Done


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

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

[kudu-CR] block id: use a better hash function

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: block_id: use a better hash function
..


Patch Set 1:

Would be great if you could synthesize a unit test that exhibits the 
improvement too, so that if we wanted to reevaluate the choice of hash function 
in the future, we wouldn't need a full cluster to do so.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore
File .gitignore:

Line 33: README.html
> Yep.
Not necessarily; I just wanted to make sure I understood the motivation for the 
inclusion.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2124. Don't hold session lock while initializing a 
TabletCopySession
..

KUDU-2124. Don't hold session lock while initializing a TabletCopySession

This patch replaces the interior mutex in TabletCopySourceSession with a
dynamic once, so that initialization can happen concurrently. This
allows initialization to happen outside of the critical section in the
tablet copy service.

Implemented a regression test that injects latency into
BeginTabletCopySession() calls.

Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/tserver/tablet_server_test_util.h
11 files changed, 249 insertions(+), 64 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..

[iwyu] kudu-specific mappings for std::tr1 entities

Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends
from TR1 (those are used in the Kudu C++ client API).

Added --max_line_length=256 IWYU option for better reporting
on the reason for header files inclusion.  If keeping the
default setting of 80 characters, IWYU sometimes omits the information
which symbols require particular header file to be included.

Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
---
M .gitignore
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/kudu.imp
4 files changed, 56 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore
File .gitignore:

Line 33: README.html
> What's this about? Do you convert README.md to README.html using a tool?
Yep.

Do you think it's not worth adding this into the .gitignore?


http://gerrit.cloudera.org:8080/#/c/7998/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

Line 47: # 
IWYU="`pwd`/../../thirdparty/clang-toolchain/bin/include-what-you-use; \
> Do you want to add --max_line_length here too?
Yep, that would be good, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore
File .gitignore:

Line 33: README.html
What's this about? Do you convert README.md to README.html using a tool?


http://gerrit.cloudera.org:8080/#/c/7998/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

Line 47: # 
IWYU="`pwd`/../../thirdparty/clang-toolchain/bin/include-what-you-use; \
Do you want to add --max_line_length here too?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] ref counted: fix move constructors to actually move

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: ref_counted: fix move constructors to actually move
..

ref_counted: fix move constructors to actually move

Ever since the move constructors for scoped_refptr were first implemented, they
didn't properly move. That is to say, they caused an unnecessary AddRef/Release
pair.

The issue is that they were passing through an rvalue-reference parameter 
without
properly using std::move(). See [1] for an explanation of this subtlety.

When combined with some other in-flight patches, this resulted in a substantial
improvement in LBM startup time, since a hashmap containing ref-counted elements
could relocate them without unnecessary reference count increments and 
decrements.
It's likely it will also improve performance elsewhere throughout Kudu.

This patch also pulls in an extra move constructor from the Chromium code base
which they claim helps Clang generate better warnings. I didn't verify that, but
figured I'd copy it while I was looking at the code.

[1] 
https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11

Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
---
M src/kudu/gutil/ref_counted.h
1 file changed, 11 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] block id: use a better hash function

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: block_id: use a better hash function
..

block_id: use a better hash function

In looking at LBM startup time I noticed that the insertion into the block
hashmap was taking substantially longer than advertised in various benchmarks.
That led me to think that our block ID patterns were causing high collision 
rates.

Swapping out the hash function for block IDs reduced the startup time
substantially.

Tested on a host with ~11M blocks across 14 drives using 'fs check'.

Before:
I0907 13:01:46.997755 21274 fs_manager.cc:335] Time spent opening block 
manager: real 114.501s  user 0.000s sys 0.002s

After:
I0907 12:58:42.863929 20320 fs_manager.cc:335] Time spent opening block 
manager: real 69.951s user 0.001s sys 0.001s

Change-Id: I30717955f962957d109a6403b55d59ab6c446a87
---
M src/kudu/fs/block_id.h
1 file changed, 7 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 5:

(1 comment)

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

Line 180:   ASSERT_OK(client_->transaction_.CommitCreatedBlocks());
> Should these be calling Finish instead of reaching into the transaction man
Finish() has some other logic which changes the state of tablet copy. I was 
trying to not include those that may effect the behavior of the tests. Does 
that make sense?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc
File README.adoc:

Line 292:  Running include-what-you-use (IWYU) against all source files in 
project
> I'm fine with keeping it in the awk script and linking to that.  This seems
SGTM -- I returned the instructions back into iwyu-filter.awk and updated them 
a little.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..

[iwyu] kudu-specific mappings for std::tr1 entities

Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends
from TR1 (those are used in the Kudu C++ client API).

Added --max_line_length=256 IWYU option for better reporting
on the reason for header files inclusion.  If keeping the
default setting of 80 characters, IWYU sometimes omits the information
which symbols require particular header file to be included.

Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
---
M .gitignore
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/kudu.imp
4 files changed, 55 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:

PS3, Line 339: // Create() should not clobber.
I think the placement of this comment might be a bit confusing. Should this 
comment be above L333, since that's where overwrite_first_flush is specified?


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

PS3, Line 97:   if (s.ok()) 
Did you consider narrowing the permissible errors here? e.g. if we hit an IO 
error when we Load(), but successfully create the in-memory cmeta, we can get 
by without addressing the error at all until the next flush. I suppose this 
probably won't affect correctness, but it might be worth thinking about what 
errors we can expect here, if you haven't already.


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

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

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc
File README.adoc:

Line 292:  Running include-what-you-use (IWYU) against all source files in 
project
> I don't think it is, but I didn't find a better place to put it.  I thought
I'm fine with keeping it in the awk script and linking to that.  This seems 
like something that we'll want to automate if it ends up being used a lot, at 
which point the directions would become significantly simpler.  A separate 
README is fine with me if you feel that's better, though.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-HasComments: Yes

[kudu-CR] Fix typo in partitioning error message

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Fix typo in partitioning error message
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Fix typo in partitioning error message

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: Fix typo in partitioning error message
..


Fix typo in partitioning error message

Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Reviewed-on: http://gerrit.cloudera.org:8080/7997
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/common/partition.cc
M src/kudu/master/master-test.cc
2 files changed, 3 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Rename tombstoned_voting-itest to tombstoned_voting-test
..


Patch Set 1:

(2 comments)

I though the '-itest' suffix stood for 'integration test'.

Maybe, rename the file to reflect the usage of the IMC via adding that into the 
suffix instead?  E.g.,

tombstoned_voting-itest.cc --> tombstoned_voting-imc-itest.cc ?

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

PS1, Line 11: prefix
suffix?


PS1, Line 12: prefix
suffix?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc
File README.adoc:

Line 292:  Running include-what-you-use (IWYU) against all source files in 
project
> Is this going to be done commonly?  If not maybe it would be better just to
I don't think it is, but I didn't find a better place to put it.  I thought 
that keeping these notes in the awk script itself was not the best place.

Maybe, I should put that a dedicated README.doc under build-support/iwyu?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4:

Just fixed IWYU failure, still good for review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc
File README.adoc:

Line 292:  Running include-what-you-use (IWYU) against all source files in 
project
Is this going to be done commonly?  If not maybe it would be better just to 
link to the previous location of the documentation.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-2134: Defer block transaction commit to the end of a 
tablet copy
..


Patch Set 5: Code-Review+1

(2 comments)

Looks good, I like how this makes the code a bit simpler (less parameters 
passed around).

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

PS5, Line 14: seesion
session


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

Line 180:   ASSERT_OK(client_->transaction_.CommitCreatedBlocks());
Should these be calling Finish instead of reaching into the transaction 
manually?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 3:

(9 comments)

Just some nits for a while -- will take a deeper 'semantic' look shortly.

http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager-stress-test.cc
File src/kudu/consensus/consensus_meta_manager-stress-test.cc:

PS3, Line 18: #include 
nit: prefer using 

I'm planning to update IWYU to give more straightforward suggestions.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.h
File src/kudu/consensus/consensus_meta_manager.h:

PS3, Line 59: ConsensusMetadataCreateMode create_mode
What if this parameter was set to ConsensusMetadataCreateMode::FLUSH_ON_CREATE 
by default?  That would allow for removal of some LOC in 
consensus_meta_manager-test.cc and other places where FLUS_ON_CREATE is the 
desired behavior and the cmeta_out is not needed.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:

PS3, Line 191: scoped_refptr cmeta;
nit: drop this and leave the last parameter of the Create() call below by 
default.


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

PS3, Line 18: #include 
nit: consider replacing with  and adding that into the list of the STD 
headers just below.

I'm sorry for the inconvenience right now, but I'm planning to update 
IWYU-related stuff accordingly to make its suggestions more straightforward.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:

PS3, Line 260: scoped_refptr cmeta;
nit: consider dropping this since it's not used in the scope and leaving the 
last parameter of the Create() call below by default.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_bootstrap-test.cc
File src/kudu/tablet/tablet_bootstrap-test.cc:

PS3, Line 146: scoped_refptr cmeta;
Nit: it seems this is not used, consider dropping it and leaving the last 
parameter for the Create() call below by default.  Also, as mentioned 
elsewhere, consider having the second-from-the-tail parameter for Create() by 
default as well.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:

PS3, Line 141: scoped_refptr cmeta;
Nit: consider dropping this since it's not used, leaving the last parameter for 
the Create() call below at its default value.


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

PS3, Line 601: scoped_refptr cmeta;
Nit: cmeta is not used in this scope, right?  Consider dropping it.


http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_source_session-test.cc
File src/kudu/tserver/tablet_copy_source_session-test.cc:

PS3, Line 157: scoped_refptr cmeta;
nit: consider dropping cmeta -- it's not used in this scope elsewhere but 
passing as the last parameter to the Create() call below, where it can be left 
by default.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix typo in partitioning error message

[Dan Burkert (Code Review)]

Hello Hao Hao, Kudu Jenkins,

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

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

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

Change subject: Fix typo in partitioning error message
..

Fix typo in partitioning error message

Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
---
M src/kudu/common/partition.cc
M src/kudu/master/master-test.cc
2 files changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..


Patch Set 1: Verified+1

Unrelated flake in RaftConsensusITest.TestTombstonedVoteAfterFailedTabletCopy

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-HasComments: No

[kudu-CR] Fix typo in partitioning error message

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Fix typo in partitioning error message
..


Patch Set 1:

It seems src/kudu/master/master-test.cc should be also updated.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Fix typo in partitioning error message

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: Fix typo in partitioning error message
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..


Patch Set 2:

(4 comments)

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

PS2, Line 7: replicas
> Nit: replica
If the cmeta doesn't exist


PS2, Line 11: Not having a cmeta
: file results in those tombstoned replicas being unable to vote in 
Raft
: leader elections
> Given the conditions you spelled out in the previous sentence, it seems lik
Due to previous versions of Kudu generating many of these kinds of tombstoned 
replicas under load, it's not that rare. Additionally, we don't want them to 
show up as FAILED in the UI, so not making them as FAILED was the primary 
motivation, and being able to vote is really something of a side benefit.


PS2, Line 27: ConsensConsensusMetadataManager
> ConsensusMetadataManager
erg, my Vim auto-wrapping keeps doing this in Git messages. I need to figure 
out why, just haven't got around to it.


PS2, Line 33: thethe
> the
same, thanks


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
Gerrit-PatchSet: 2
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: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: Fix flakiness of ts_tablet_manager_itest 
TestFailedTabletsAreReplaced
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't 
exist at startup
..

KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup

It is possible for tombstoned replicas to legitimately not have a cmeta
file as a result of crashing during a first tablet copy, or failing a
tablet copy operation in an older version of Kudu. Not having a cmeta
file results in those tombstoned replicas being unable to vote in Raft
leader elections. We remedy this by creating a cmeta object (with an
empty config) at startup time. The empty config is safe for a tombstoned
replica, because the config doesn't affect a replica's ability to vote
in a leader election. Additionally, if the tombstoned replica were ever
to be overwritten by a tablet copy operation, that would also result in
overwriting the config stored in the local cmeta with a valid Raft
config. Finally, all of this assumes that the nonexistence of a cmeta
file guarantees that the replica has never voted in a leader election.

As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE
flag, meaning that it will only be flushed to disk if the replica ever
votes.

The following changes had to be made to ConsensusMetadata and the
ConsensusMetadataManager to support the above functionality:

* Enable deferred flush on Create() by defining a flag called
  NO_FLUSH_ON_CREATE
* Simplify the interface controlling whether a Flush() is allowed to
  overwrite (clobber) an existing file by encapsulating that logic in
  the implementation, instead of the public interface to
  ConsensusMetadata. When a cmeta is instantiated via
  ConsensusMetadata::Create(), the next Flush() is not allowed to
  overwrite an existing file. In every other case, Flush() is allowed to
  overwrite an existing file.

The following tests have been added:

* A unit test for NO_FLUSH_ON_CREATE.
* A test that crashes the target of a tablet copy after writing the
  superblock and before writing the cmeta file. The tablet server is
  restarted and the replica is expected to be able to vote while
  tombstoned.

Previously-written tests that verify ConsensusMetadata::Create() will
not clobber an existing file still pass.

Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
---
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/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_meta_manager-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
16 files changed, 357 insertions(+), 38 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b
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-Reviewer: Todd Lipcon 

[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [iwyu] kudu-specific mappings for std::tr1 entities
..

[iwyu] kudu-specific mappings for std::tr1 entities

Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends
from TR1 (those are used in the Kudu C++ client API).  Also, moved the
instructions for running a non-incremental IWYU verification
(i.e. running the tool against every C++ source file in the project)
from build-support/iwyu/iwyu-filter.awk file into top-level README.adoc

The additional --max_line_length=256 IWYU option allows for better
reporting of the reason for header files inclusion.  If keeping the
default setting of 80 characters, IWYU sometimes skipped the information
which symbols require particular header file to be included.

Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84
---
M .gitignore
M README.adoc
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/kudu.imp
5 files changed, 87 insertions(+), 46 deletions(-)


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

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

[kudu-CR] Fix typo in partitioning error message

[Dan Burkert (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Fix typo in partitioning error message
..

Fix typo in partitioning error message

Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
---
M src/kudu/common/partition.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 3:

(2 comments)

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

PS3, Line 10: it
> is
Done


http://gerrit.cloudera.org:8080/#/c/7981/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS3, Line 2582: do
> go?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 144 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Add tablet state summary metrics

[Will Berkeley (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
..

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 308 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop

[Adar Dembo (Code Review)]

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: KUDU-2137: protect against concurrent schema version change and 
tablet drop
..

KUDU-2137: protect against concurrent schema version change and tablet drop

Try as I might, I could not reproduce the failure in the bug report. I
looped the failed test several thousand times. I also looped the entire
alter_table-test suite a thousand times. Finally, I wrote a unit test that
hammers one table with concurrent add column, add partition, and drop
partition operations. Nothing worked.

So, here's my best guess at what's going on: if a tablet is dropped
while the master is processing its report, it's conceivable that we could
wind up in TabletInfo::set_reported_schema_version() with the table spinlock
held just after TableInfo::AddRemoveTablets() dropped the tablet. This would
cause us to decrement the tablet's "old" schema version from the table's
count map twice: once when dropping the tablet and a second time in
set_reported_schema_version().

The fix is straight-forward: after acquiring both spinlocks, double check
that the tablet is still a member of the table. But, the extra locking
needed to do so feels so very wrong.

Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a
---
M src/kudu/master/catalog_manager.cc
1 file changed, 16 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] catalog manager: fix unprotected data access in TableInfo::AddRemoveTablets

[Adar Dembo (Code Review)]

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: catalog_manager: fix unprotected data access in 
TableInfo::AddRemoveTablets
..

catalog_manager: fix unprotected data access in TableInfo::AddRemoveTablets

RWCLock::HasWriteLock never worked properly because last_writer_tid_ wasn't
reset when the lock was released. Well, it worked properly (though it's far
less useful) in non-DEBUG builds, but then the various HasWriteLock DCHECKs
are compiled out. Who knew?

When fixed, TableInfo::AddRemoveTablets broke; we had been reading some
tablet metadata without first acquiring a lock! This was frustrasting to
address because it meant retreading over the most error-prone aspect of the
catalog manager: for operations that require several writes in order to
"publish" their results, in what order should those writes occur? I tried my
best to get this right, but who knows how I did...

Change-Id: Ifcc5d3a9b985210f1fdd6f0495326fa3eb707841
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/util/cow_object.h
M src/kudu/util/rwc_lock.cc
M src/kudu/util/rwc_lock.h
6 files changed, 60 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifcc5d3a9b985210f1fdd6f0495326fa3eb707841
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Fix flakiness of ts_tablet_manager_itest 
TestFailedTabletsAreReplaced
..


Patch Set 2: Code-Review+1

Will defer to Mike.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] rename suicide on eio flag to crash on eio

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: rename suicide_on_eio flag to crash_on_eio
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c83156865b0f8ac80a20701558119ace6c05399
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] rename suicide on eio flag to crash on eio

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: rename suicide_on_eio flag to crash_on_eio
..

rename suicide_on_eio flag to crash_on_eio

The name `suicide_on_eio` seems a bit controversial/insensitive. As we
move forward in documenting its usage with disk failures, it's probably
best rename it.

Change-Id: I6c83156865b0f8ac80a20701558119ace6c05399
---
M docs/release_notes.adoc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
8 files changed, 17 insertions(+), 17 deletions(-)


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

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

[kudu-CR] docs: update disk failure recovery notes

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: docs: update disk failure recovery notes
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7984/1/docs/administration.adoc
File docs/administration.adoc:

Line 597: empty all of the server's existing directories. For example, if a 
tablet server
> do you think a WARNING bar is appropriate here? I wonder if someone would f
Done


PS1, Line 608: along with any newly-added data
 : directories
> you mean 'and new data directories have been created with appropriate permi
Done


PS1, Line 630: suicide_on_eio=false`. When set, tablets with data on a failed 
disk
 : will not be opened and will be re-replicated as necessary
> given that we are currently defaulting to spreading tablets across all disk
Fair point about striping. Made it less of a point.

Also changed to --crash_on_eio; hopefully less controversial and made this 
patch dependent on the separate patch.


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

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

[kudu-CR] docs: update disk failure recovery notes

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: docs: update disk failure recovery notes
..

docs: update disk failure recovery notes

Adds more context around disk failures and separates out steps to
rebuild a server with a different directory configuration.

Change-Id: I732286d0f56f7a15705ad544fc7dfc426287714e
---
M docs/administration.adoc
1 file changed, 38 insertions(+), 19 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I732286d0f56f7a15705ad544fc7dfc426287714e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:

PS2, Line 184: an
> a
Done


http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

Line 2590:   if (report.has_error()) {
> I'm curious as to why this block was moved down here and not just before th
I'd wanted it to be slightly tailored for the case I expect is more common 
where report.has_consensus_state() == true so I put it first. It's not really 
an important move, so I'll move it back, sorry about that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes