subject:"\[kudu\-CR\] Create ConsensusMetadataManager"

Mike Percy has submitted this change and it was merged.

Change subject: Create ConsensusMetadataManager
..


Create ConsensusMetadataManager

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

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

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

This patch also changes the contract of
ConsensusMetadata::DeleteOnDiskData() to return NotFound if a
nonexistent cmeta is deleted so that ConsensusMetadataManager::Delete()
can provide that same contract, and modifies the single callsite (in
TSTabletManager) to account for that change.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Reviewed-on: http://gerrit.cloudera.org:8080/7191
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 786 insertions(+), 210 deletions(-)

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



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

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

[kudu-CR] Create ConsensusMetadataManager

2017-07-12 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..

Patch Set 18: Code-Review+2

(3 comments)

Probably you want to get more feedback from David and Todd, but so far it LGTM.

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

PS17, Line 98: kNumOpsPerThread = 1000
> Yeah it's pretty fast:
ok, this is great.

PS17, Line 187:
> Yeah but that is equivalent to load(std::memory_order_seq_cst) which I beli
All right, if you want it here because of that, that's OK. I just thought it
might be easier to read without memory order spec since performance is not such
an issue here.

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

PS17, Line 50: DCHECK(cmeta_out);
> I thought about it but it wasn't needed by any existing code. Most of the t
That's fine -- let's keep it then.

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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

This patch also changes the contract of
ConsensusMetadata::DeleteOnDiskData() to return NotFound if a
nonexistent cmeta is deleted so that ConsensusMetadataManager::Delete()
can provide that same contract, and modifies the single callsite (in
TSTabletManager) to account for that change.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 786 insertions(+), 210 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 17:

(9 comments)

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

PS17, Line 73: Status ConsensusMetadata::DeleteOnDiskData(FsManager* 
fs_manager, const string& tablet_id) {
 :   string cmeta_path = 
fs_manager->GetConsensusMetadataPath(tablet_id);
 :   
RETURN_NOT_OK_PREPEND(fs_manager->env()->DeleteFile(cmeta_path),
 : Substitute("Unable to delete consensus 
metadata file for tablet $0",
 :tablet_id));
 :   return Status::OK();
 : }
> style nit: consider re-ordering the methods to match their declaration orde
I avoided moving everything around to make this an easy change to review, and 
the file is already not very strict in that regard (surely my fault for this 
reason) but I'll move these 3 methods.


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

PS17, Line 95: TEST_F(ConsensusMetadataManagerStressTest, 
CreateLoadDeleteTSANTest) {
> Should it be run only when KUDU_ALLOW_SLOW_TESTS is enabled?  60 seconds se
60 seconds is just the maximum timeout, it only takes about a second with 
stress. See below.


PS17, Line 98: kNumOpsPerThread = 1000
> Did you try to run it under ASAN configuration with --stress-cpu-threads=8?
Yeah it's pretty fast:

  + ../../build-support/run-test.sh 
  ./bin/consensus_meta_manager-stress-test 
  consensus_meta_manager-stress-test --stress_cpu_threads=8
  Running consensus_meta_manager-stress-test, redirecting 
  output into 
/home/mpercy/src/kudu/build/asan/test-logs/consensus_meta_manager-stress-test.txt
 (attempt 1/1)

  real0m1.016s
  user0m5.716s
  sys 0m0.368s
  + RETCODE=0


PS17, Line 141: scoped_refptr cmeta;
> nit: is it possible to make this private for the 'kLoad' case scope only?
Done


PS17, Line 163: break;
> Is it intentional that the previous state is not stored in the prev_states 
Yes, done. Actually I changed this to use a bool in that map to indicate 
existence which was really the intent there.


PS17, Line 187: ops_performed.load(std::memory_order_relaxed)
> I think it's possible to use just 'ops_performed' here.
Yeah but that is equivalent to load(std::memory_order_seq_cst) which I believe 
would be more expensive than std::memory_order_relaxed because it will generate 
a memory fence.


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

PS17, Line 43: virtual
> nit: consider dropping 'virtual' since override is 'present'.
Doh. Tough to break this habit learned from TR1 which was virtual void SetUp() 
OVERRIDE {


PS17, Line 104: Status s = cmeta_manager_->Create(kTabletId, config_, 
kInitialTerm, );
> Is it worth adding a check that it's not possible to create a meta with dif
That code path already has coverage in the unit test 
ConsensusMetadataTest.TestMergeCommittedConsensusStatePB so seems like overkill 
to retest it


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

PS17, Line 50:  DCHECK(cmeta_out);
> nit: this a little bit not symmetric -- Create() allows to pass null pointe
I thought about it but it wasn't needed by any existing code. Most of the time 
we call Load() we want to read something from the ConsensusMetadata file soon 
after that, so I kind of like it the way it is. But if you feel strongly about 
it I will change it.


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-07-12 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 17:

(9 comments)

some nits.

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

PS17, Line 73: Status ConsensusMetadata::DeleteOnDiskData(FsManager* 
fs_manager, const string& tablet_id) {
 :   string cmeta_path = 
fs_manager->GetConsensusMetadataPath(tablet_id);
 :   
RETURN_NOT_OK_PREPEND(fs_manager->env()->DeleteFile(cmeta_path),
 : Substitute("Unable to delete consensus 
metadata file for tablet $0",
 :tablet_id));
 :   return Status::OK();
 : }
style nit: consider re-ordering the methods to match their declaration order in 
the .h file (feel free to ignore if it's not a common practice right now).


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

PS17, Line 95: TEST_F(ConsensusMetadataManagerStressTest, 
CreateLoadDeleteTSANTest) {
Should it be run only when KUDU_ALLOW_SLOW_TESTS is enabled?  60 seconds seems 
like a long time to run it every time.


PS17, Line 98: kNumOpsPerThread = 1000
Did you try to run it under ASAN configuration with --stress-cpu-threads=8?  It 
would be nice to check that it's not about to timeout in that case.


PS17, Line 141: scoped_refptr cmeta;
nit: is it possible to make this private for the 'kLoad' case scope only?


PS17, Line 163: break;
Is it intentional that the previous state is not stored in the prev_states in 
thise case?  If so, could you please add a comment explaining why?


PS17, Line 187: ops_performed.load(std::memory_order_relaxed)
I think it's possible to use just 'ops_performed' here.


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

PS17, Line 43: virtual
nit: consider dropping 'virtual' since override is 'present'.


PS17, Line 104: Status s = cmeta_manager_->Create(kTabletId, config_, 
kInitialTerm, );
Is it worth adding a check that it's not possible to create a meta with 
different initial term?


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

PS17, Line 50:  DCHECK(cmeta_out);
nit: this a little bit not symmetric -- Create() allows to pass null pointer 
for cmeta_out.  Maybe, allow passing nullptr  for cmeta_out in this case as 
well?  That could get rid of some extra lines in the tests as well.


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 17:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7191/17/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:

Line 113:   local_peer_pb_(std::move(local_peer_pb)),
> warning: passing result of std::move() as a const reference argument; no mo
existing issue; ignoring for this patch


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

This patch also changes the contract of
ConsensusMetadata::DeleteOnDiskData() to return NotFound if a
nonexistent cmeta is deleted so that ConsensusMetadataManager::Delete()
can provide that same contract, and modifies the single callsite (in
TSTabletManager) to account for that change.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 751 insertions(+), 178 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-07-06 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 16:

(1 comment)

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

Line 67:   Status Delete(const std::string& tablet_id);
still not quite sure what the "expected" concurrency is here in the class. 
Specifically, what happens with the following:

thread A: local_ref = Load("foo")
thread B: Delete("foo")
thread B: local_ref2 = Create("foo")
thread A: local_ref->Flush()

now we have two different refs pointing to the same instance.

Do we need to invalidate the existing 'local_ref' in Delete so that we don't 
get into the situation where we have multiple ConsensusMetadata instances alive 
for the same tablet?

Put another way, I agree that this class is thread-safe from the perspective of 
crashes, etc, but I am not sure what the expected _usage_ of it is, and 
how/whether it is protecting from different unexpected interleavings


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-07-06 Thread Mike Percy (Code Review)

Hello Alexey Serbin,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

This patch also changes the contract of
ConsensusMetadata::DeleteOnDiskData() to return NotFound if a
nonexistent cmeta is deleted so that ConsensusMetadataManager::Delete()
can provide that same contract, and modifies the single callsite (in
TSTabletManager) to account for that change.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 877 insertions(+), 178 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-07-06 Thread Mike Percy (Code Review)

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 15:

(10 comments)

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

Line 106:   alarm(60);
> Add a comment explaining why we do this.
Done


Line 115: Status s;
> Not using this? Shouldn't we be doing something with the results of Create(
See the comment at the top of the test. This test is just here to trigger TSAN 
if there are any data races detected. I don't want to hold the lock during 
cmeta manager ops (it would defeat the purpose of the test) but if I don't then 
it's racy creating / deleting items from a list. So I'm not sure it would 
really help much.

One thing I could do is keep track of created / loaded instances but that's 
already covered by 
ConsensusMetadataManagerStressTest.TestConcurrentCreateLoadSameTablet. This 
test just adds multiple concurrent tablets into the mix, so I don't think it 
helps.


Line 136:   alarm(0);
> Set this up via MakeScopedCleanup just after the alarm(60).
Done


Line 139: void ConsensusMetadataManagerStressTest::DoLoad(Barrier* barrier, int 
thread_num,
> Could you convert this into a lambda? There'd be less argument marshalling 
I originally had it like that but TSAN flagged it. Turns out I was capturing 
thread_num by reference; passing by value works. Done.


Line 150:   lock_guard l(lock_);
> Does cmeta_ptrs need synchronization even though you've preallocated it up-
Ah, you're right. I'll change it.


Line 158:   uintptr_t cmeta_ptrs[kNumThreads];
> Why uintptr_t? Why not store them as ConsensusMetadata* and do a cast at th
I originally thought it would be flagged by ASAN but I was able to do it 
without problems. Done.


Line 168:   alarm(60);
> Comment + MakeScopedCleanup.
Done


Line 183:   // Now check that all the values are the same.
> Another way to test this would be to add metrics to the Manager that track 
I'm not sure hit / miss metrics are very useful in production since 
ConsensusMetadataManager caches everything created or loaded for its lifetime. 
It's not like entries are getting evicted from the cache and then reloaded.


Line 230:   // Keep track of the addresses of created and loaded cmeta 
instances.
> Again, would prefer that you did the casting as late as possible (i.e. duri
Done


Line 234:   alarm(60);
> Comment + MakeScopedCleanup.
Done


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-06-30 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 15:

(10 comments)

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

Line 106:   alarm(60);
Add a comment explaining why we do this.


Line 115: Status s;
Not using this? Shouldn't we be doing something with the results of 
Create()/Load()/Delete()?

You could also lock tablet_ids and add/remove entries as Create/Load() or 
Delete() succeed.


Line 136:   alarm(0);
Set this up via MakeScopedCleanup just after the alarm(60).


Line 139: void ConsensusMetadataManagerStressTest::DoLoad(Barrier* barrier, int 
thread_num,
Could you convert this into a lambda? There'd be less argument marshalling so 
less net code, I think.


Line 150:   lock_guard l(lock_);
Does cmeta_ptrs need synchronization even though you've preallocated it 
up-front? In fact, you don't even need to pass the array in; you can pass a 
double pointer to the exact appropriate entry into each thread.


Line 158:   uintptr_t cmeta_ptrs[kNumThreads];
Why uintptr_t? Why not store them as ConsensusMetadata* and do a cast at the 
end, when you're comparing? Seems cleaner.


Line 168:   alarm(60);
Comment + MakeScopedCleanup.


Line 183:   // Now check that all the values are the same.
Another way to test this would be to add metrics to the Manager that track 
cache misses/hits, and to test those. That would be nice for other reasons 
(i.e. now we have new metrics we can publish).


Line 230:   // Keep track of the addresses of created and loaded cmeta 
instances.
Again, would prefer that you did the casting as late as possible (i.e. during 
comparisons).


Line 234:   alarm(60);
Comment + MakeScopedCleanup.


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-06-30 Thread Mike Percy (Code Review)

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 15: Verified+1

Overriding failure due to KUDU-2059

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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

This patch also changes the contract of
ConsensusMetadata::DeleteOnDiskData() to return NotFound if a
nonexistent cmeta is deleted so that ConsensusMetadataManager::Delete()
can provide that same contract, and modifies the single callsite (in
TSTabletManager) to account for that change.

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 895 insertions(+), 178 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 896 insertions(+), 178 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 11:

(1 comment)

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

Line 84: while (ContainsKey(op_in_progress_, tablet_id)) {
> Mike: what if the order was:
I didn't see the comments you guys posted until just now; was working on a MT / 
stress test that I just posted.

Adar: Ah, thanks for the suggestion; that is simpler than what I'm doing in Rev 
12. Will update it in a sec.

Todd: I don't want to hold the global lock during ConsensusMetadata::Delete(), 
but I also want to block concurrent loaders from reading it from disk and 
reloading the cache while while I delete the underlying file. I think we need a 
condition variable to do that.


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-stress-test.cc
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
26 files changed, 901 insertions(+), 178 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-06-29 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 11:

(1 comment)

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

Line 84: while (ContainsKey(op_in_progress_, tablet_id)) {
> Oh, you are right. I want concurrent Load() to avoid the condition variable
another thought how to make this simpler:

what if we had a map>?

I think you'd need to make Promise be movable, but then I think the could could 
be a lot simpler -- on any lookup, if it's not there, insert a promise, and 
then release the lock. then fulfill the promise. If you see a promise there, 
then just call Get() on it to wait for a concurrent loader/creator to finish 
its creation? Would that work?


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 11:

(6 comments)

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

Line 36: class ConsensusMetadataManagerTest : public KuduTest {
> Would be good to also have a "stress" test that spams a ConsensusMetadataMa
I'll add this.


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

Line 33: using std::lock_guard;
> Nit: out of order.
Thanks for the catch.


Line 59:   RETURN_NOT_OK(ConsensusMetadata::Create(fs_manager_, tablet_id, 
fs_manager_->uuid(),
> I presume this will fail if there's already an on-disk cmeta?
Yes.


Line 84: while (ContainsKey(op_in_progress_, tablet_id)) {
> Hmm, what if we enter this loop because there's a concurrent Load() on the 
Oh, you are right. I want concurrent Load() to avoid the condition variable in 
a steady state so I'll add an additional check down below when we lock again on 
L97.


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

Line 52:   // 'cmeta_out' is optional.
> Nit: I think this is implicit due to the default value of cmeta_out.
K, I'll remove it.


PS11, Line 65: or if the instance cannot be found.
> This is a little unusual; why not return NotFound and let callers decide wh
That's a fair point. Let me see what I can do about this.


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

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

[kudu-CR] Create ConsensusMetadataManager

2017-06-29 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 11:

(6 comments)

Just reviewed the consensus_meta_manager files.

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

Line 36: class ConsensusMetadataManagerTest : public KuduTest {
Would be good to also have a "stress" test that spams a 
ConsensusMetadataManager with requests from multiple threads.


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

Line 33: using std::lock_guard;
Nit: out of order.


Line 59:   RETURN_NOT_OK(ConsensusMetadata::Create(fs_manager_, tablet_id, 
fs_manager_->uuid(),
I presume this will fail if there's already an on-disk cmeta?


Line 84: while (ContainsKey(op_in_progress_, tablet_id)) {
Hmm, what if we enter this loop because there's a concurrent Load() on the same 
tablet we're looking for? Shouldn't we check the cache again when we exit the 
loop? Maybe reoder the FindOrNull to happen after this loop?


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

Line 52:   // 'cmeta_out' is optional.
Nit: I think this is implicit due to the default value of cmeta_out.


PS11, Line 65: or if the instance cannot be found.
This is a little unusual; why not return NotFound and let callers decide 
whether it's OK for them or not?


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
25 files changed, 609 insertions(+), 171 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 605 insertions(+), 158 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 603 insertions(+), 160 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 7:

(1 comment)

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

PS7, Line 51: lock_guard l(lock_);
> Good point. But I guess it's moot, I'll try to simplify.
Done


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

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

[kudu-CR] Create ConsensusMetadataManager

Hello Kudu Jenkins,

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

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

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

Change subject: Create ConsensusMetadataManager
..

Create ConsensusMetadataManager

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

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

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

Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 602 insertions(+), 155 deletions(-)


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

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

[kudu-CR] Create ConsensusMetadataManager

Mike Percy has posted comments on this change.

Change subject: Create ConsensusMetadataManager
..


Patch Set 7:

(1 comment)

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

Line 44: // Ensure we don't interleave with other ops on this tablet.
> No, they are mutexed in this implementation.
Hmm. I was about to post a simplification but it was too weak and didn't 
reliably maintain a single cmeta instance for concurrent Create() and Load(). 
So I am going to stick with the version posted here in Rev 7.


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

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

[kudu-CR] Create ConsensusMetadataManager