You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/06/15 17:32:07 UTC
[kudu-CR] Create ConsensusMetadataService
Hello David Ribeiro Alves, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7191
to review the following change.
Change subject: Create ConsensusMetadataService
......................................................................
Create ConsensusMetadataService
This patch creates an API for a "service" that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:
1) This service 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 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.
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_service-test.cc
A src/kudu/consensus/consensus_meta_service.cc
A src/kudu/consensus/consensus_meta_service.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, 447 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/1
--
To view, visit http://gerrit.cloudera.org:8080/7191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7191
to look at the new patch set (#6).
Change subject: Create ConsensusMetadataManager
......................................................................
Create ConsensusMetadataManager
This patch creates an API for a class that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:
1) An instance of this class can be plumbed throughout the various
classes that deal with reading, creating, and modifying consensus
metadata so that we don't have to pass the individual cmeta instances
around.
2) It provides additional abstraction and flexibility that will make it
easier to change the underlying implementation of ConsensusMetadata in
the future as needed, perhaps if we wanted to make them log structured
and group committed across all replicas on a tablet server.
Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 574 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/6
--
To view, visit http://gerrit.cloudera.org:8080/7191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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<tablet_id, Promise<shared_ptr<ConsensusMetadata>>>?
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Create ConsensusMetadataManager
......................................................................
Patch Set 2:
(4 comments)
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
> in that case, if a load races with a create, can it see partially written d
No, they are mutexed in this implementation.
However I see that this impl is confusing for reviewers. So I am going to weaken the semantics a bit and simplify the impl a lot.
PS7, Line 51:
> nit: can't you just lock in CancelOpInProgress?
Good point. But I guess it's moot, I'll try to simplify.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
PS2, Line 169: meta
> maybe just on the public method then.
Not excited about this because it's a slippery slope. But OK - I'll do it in this file. A separate patch makes more sense for a pervasive rename.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS2, Line 1067: const scoped_refptr<consensus::ConsensusMetadataService>&
> yeah, but the caller might then think that this method increases the refcou
I don't see a problem with that. This method only takes a constref and returns when it's done so the caller can drop the ref after this method returns. From my perspective it would be significantly more confusing if we passed a bare ptr here.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<ConsensusMetadata> 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, &cmeta);
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#5).
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, 573 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/5
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Create ConsensusMetadataManager
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Create ConsensusMetadataManager
......................................................................
Patch Set 2:
(4 comments)
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
> > Isn't interleaving on Create() already a disallowed operation?
in that case, if a load races with a create, can it see partially written data? same with a delete. it seems like concurrent creates/deletes can't happen, but what if a load happens while data is being mutated on disk?
PS7, Line 51:
nit: can't you just lock in CancelOpInProgress?
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
PS2, Line 169: meta
> I did consider it but I think if we want to do that it should be a separate
maybe just on the public method then.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS2, Line 1067: const scoped_refptr<consensus::ConsensusMetadataService>&
> Why? Passing it this way provides useful information, i.e. that this is ref
yeah, but the caller might then think that this method increases the refcount, which it doesn't, and that it no longer need to keep a refcount itself.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#3).
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, 573 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/3
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Create ConsensusMetadataManager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo 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)) {
> another thought how to make this simpler:
Mike: what if the order was:
1. Take the lock
2. Run the check op_in_progress_ and wait on cv loop
3. FindOrNull on cmeta_cache_ and early return
4. InsertOrDie(op_in_progress, ...),
5. Release lock
6. The rest of Load().
In the steady state, op_in_progress_ is empty so the fast path is just a lock acquisition and two map lookups. No condvar interaction at all.
Todd: I assume you also mean to do away with the op_in_progress map? If that's what you meant, I don't see how to make concurrent Delete() safe, since presumably it involves erasing from the promise map, and I don't see any way to order the actual on-disk deletion and the erasure in such a way that you'd block another thread trying to Delete() your tablet.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<simple_spinlock> 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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Create ConsensusMetadataManager
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7191/7/src/kudu/consensus/consensus_meta_manager.cc
File src/kudu/consensus/consensus_meta_manager.cc:
Line 44: // Ensure we don't interleave with other ops on this tablet.
> Isn't interleaving on Create() already a disallowed operation?
> Isn't interleaving on Create() already a disallowed operation?
Well... ConsensusMetadata::Create() will fail for the 2nd caller, if that's what you mean.
> Even with this locking, if you had interleaved with another Create or Load, you'd just crash on the InsertOrDie, right?
No, I don't think so. The 2nd thread would have to wait until the ScopedCleanup clears op_in_progress_[tablet_id] and then it can enter the main block. If, at that time, the cmeta exists, it will get an error back from ConsensusMetadata::Create().
> And if you are concurrent with a Delete(), then you wouldn't crash but still seems like invalid usage of the API.
I think this does the right thing with multiple concurrent ops, such as 2 concurrent calls Create() and one to Delete(). However, all of that stuff should probably not be going on at once.
> What are the legal interleavings that actually could/should happen in practice?
Good question. If we assume Create() and Delete() calls are externally mutexed for a given tablet then we can make it simpler. In the current Kudu code base, that would be a safe assumption due to the TSTabletManager "tablet state transition" reservation mechanism.
> Maybe something simpler is possible here, eg allowing concurrent Load() to potentially do extra work and "first loader wins" in the case of a race?
I think concurrent Load() would be straightforward to simplify as long as only one ConsensusMetadata instance survives at the end. I guess the question is whether adding additional constraints on the API s.t. concurrent Create() and Delete() are illegal will come back to bite us later since it may be hard to detect when it occurs. But maybe it's worth getting rid of this additional complexity.
--
To view, visit http://gerrit.cloudera.org:8080/7191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataService
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Create ConsensusMetadataService
......................................................................
Patch Set 2:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service-test.cc
File src/kudu/consensus/consensus_meta_service-test.cc:
PS2, Line 31: ConsensusMetadataServiceTest
> how does overwrite behave? is it allowed? likely should have a test.
Done
PS2, Line 38: OVERRIDE
> use c++11 style overrides
Done
PS2, Line 68: ASSERT_OK(cmeta_service_->Load(kTabletId, &cmeta));
> no testing that it matches what you expect?
Done
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service.cc
File src/kudu/consensus/consensus_meta_service.cc:
PS2, Line 42: RETURN_NOT_OK(ConsensusMetadata::Create(fs_manager_, tablet_id, fs_manager_->uuid(),
: config, current_term, cmeta_out));
> is it strictly necessary to do the create under the lock?
I was thinking it would be a rare enough operation but I suppose it was a bit too simplistic. I've updated this with a scheme that involves a map and a condition variable to get callers to block on outstanding operations on the cmeta instance they are accessing instead of holding the lock across the whole set while performing I/O.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service.h
File src/kudu/consensus/consensus_meta_service.h:
PS2, Line 33: ConsensusMetadataService
> class header doc
Done
PS2, Line 39: // On success, returns OK.
> nit: we generally omit this (otherwise all the methods would have to have i
Done
PS2, Line 51: // Delete the ConsensusMetadata instance keyed by 'tablet_id'.
> is the delete permanent, or do we leave something lying around? can you tal
Permanent. Done
PS2, Line 53: // Returns an error if the cmeta instance exists but cannot be deleted.
> why wouldn't we be able to delete it?
Could be a FS permissions issue or something like that.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:
PS2, Line 575:
> nit no longer need to leave this space
Done
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/integration-tests/tablet_copy-itest.cc
File src/kudu/integration-tests/tablet_copy-itest.cc:
PS2, Line 56: using kudu::consensus::ConsensusMetadataService;
> nit: "consensus" goes after "client" :)
ah, right :)
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/integration-tests/ts_recovery-itest.cc
File src/kudu/integration-tests/ts_recovery-itest.cc:
Line 25: #include "kudu/consensus/log-test-base.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
Line 78:
> spurious change
Done
PS2, Line 169: meta
> consider renaming this param to "tablet_meta"
I did consider it but I think if we want to do that it should be a separate patch. It's literally everywhere.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:
PS2, Line 24: #include "kudu/common/wire_protocol.h"
> interesting, I'd think this one came before the one above, but apparently n
Yeah... llvm-tidy's lexicographic include sort order is mostly fine but I don't like this quirk either (- before . before _ in ASCII)
Line 62: using consensus::OpIdEquals;
> warning: using decl 'OpIdEquals' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
PS2, Line 104: cmeta_service_(std::move(cmeta_service)),
> I see you're using move there but const refs elsewhere? any particular reas
In this case we are holding a ref to the cmeta service so as long as that's the case this makes sense.
As of C++11 this approach is easier for the compiler to optimize from what I understand. There are a bunch of articles on this around, e.g. http://www.codesynthesis.com/~boris/blog/2012/06/19/efficient-argument-passing-cxx11-part1/
In general we are trying to prefer pass-by-value + move over pass-by-constref + copy. Of course, in cases where you don't plan to copy, pass-by-constref probably still makes more sense.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
PS2, Line 74: scoped_refptr
> use const ref
this is legit because TabletCopyClient keeps a ref to ConsensusMetadataService
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS2, Line 1067: const scoped_refptr<consensus::ConsensusMetadataService>&
> if you don't need to increase the refcount consider passing a raw pointer
Why? Passing it this way provides useful information, i.e. that this is refcounted, and passing a const ref doesn't increment the refcount so there is no perf hit.
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:
PS2, Line 289: const
> does the const server a purpose here?
It simply enforces the invariant that it's set (actually, it's created) in the constructor and that the instance is never replaced, so we get a per-TSTabletManager "singleton".
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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)
real 0m1.016s
user 0m5.716s
sys 0m0.368s
+ RETCODE=0
PS17, Line 141: scoped_refptr<ConsensusMetadata> 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, &cmeta);
> 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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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<simple_spinlock> 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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<Mutex> 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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Create ConsensusMetadataManager
......................................................................
Patch Set 7:
(1 comment)
just looked at the manager impl for starters
http://gerrit.cloudera.org:8080/#/c/7191/7/src/kudu/consensus/consensus_meta_manager.cc
File src/kudu/consensus/consensus_meta_manager.cc:
Line 44: // Ensure we don't interleave with other ops on this tablet.
Isn't interleaving on Create() already a disallowed operation?
Even with this locking, if you had interleaved with another Create or Load, you'd just crash on the InsertOrDie, right?
And if you are concurrent with a Delete(), then you wouldn't crash but still seems like invalid usage of the API.
What are the legal interleavings that actually could/should happen in practice? Maybe something simpler is possible here, eg allowing concurrent Load() to potentially do extra work and "first loader wins" in the case of a race?
--
To view, visit http://gerrit.cloudera.org:8080/7191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataService
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#2).
Change subject: Create ConsensusMetadataService
......................................................................
Create ConsensusMetadataService
This patch creates an API for a "service" that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:
1) This service 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 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.
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_service-test.cc
A src/kudu/consensus/consensus_meta_service.cc
A src/kudu/consensus/consensus_meta_service.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, 452 insertions(+), 121 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/2
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Create ConsensusMetadataManager
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7191
to look at the new patch set (#7).
Change subject: Create ConsensusMetadataManager
......................................................................
Create ConsensusMetadataManager
This patch creates an API for a class that manages consensus
metadata. This abstracts out the management of ConsensusMetadata files,
providing the following benefits:
1) An instance of this class can be plumbed throughout the various
classes that deal with reading, creating, and modifying consensus
metadata so that we don't have to pass the individual cmeta instances
around.
2) It provides additional abstraction and flexibility that will make it
easier to change the underlying implementation of ConsensusMetadata in
the future as needed, perhaps if we wanted to make them log structured
and group committed across all replicas on a tablet server.
Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
A src/kudu/consensus/consensus_meta_manager-test.cc
A src/kudu/consensus/consensus_meta_manager.cc
A src/kudu/consensus/consensus_meta_manager.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
24 files changed, 574 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7191/7
--
To view, visit http://gerrit.cloudera.org:8080/7191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia30c05dd0feec2b7530205f4d17dfc079a1c3451
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
---
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Create ConsensusMetadataService
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Create ConsensusMetadataService
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service-test.cc
File src/kudu/consensus/consensus_meta_service-test.cc:
PS2, Line 31: ConsensusMetadataServiceTest
how does overwrite behave? is it allowed? likely should have a test.
PS2, Line 38: OVERRIDE
use c++11 style overrides
PS2, Line 68: ASSERT_OK(cmeta_service_->Load(kTabletId, &cmeta));
no testing that it matches what you expect?
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service.cc
File src/kudu/consensus/consensus_meta_service.cc:
PS2, Line 42: RETURN_NOT_OK(ConsensusMetadata::Create(fs_manager_, tablet_id, fs_manager_->uuid(),
: config, current_term, cmeta_out));
is it strictly necessary to do the create under the lock?
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/consensus_meta_service.h
File src/kudu/consensus/consensus_meta_service.h:
PS2, Line 33: ConsensusMetadataService
class header doc
PS2, Line 39: // On success, returns OK.
nit: we generally omit this (otherwise all the methods would have to have it)
PS2, Line 51: // Delete the ConsensusMetadata instance keyed by 'tablet_id'.
is the delete permanent, or do we leave something lying around? can you talk a bit about that?
PS2, Line 53: // Returns an error if the cmeta instance exists but cannot be deleted.
why wouldn't we be able to delete it?
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:
PS2, Line 575:
nit no longer need to leave this space
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/integration-tests/tablet_copy-itest.cc
File src/kudu/integration-tests/tablet_copy-itest.cc:
PS2, Line 56: using kudu::consensus::ConsensusMetadataService;
nit: "consensus" goes after "client" :)
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
Line 78:
spurious change
PS2, Line 169: meta
consider renaming this param to "tablet_meta"
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:
PS2, Line 24: #include "kudu/common/wire_protocol.h"
interesting, I'd think this one came before the one above, but apparently not
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
PS2, Line 104: cmeta_service_(std::move(cmeta_service)),
I see you're using move there but const refs elsewhere? any particular reason?
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
PS2, Line 74: scoped_refptr
use const ref
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS2, Line 1067: const scoped_refptr<consensus::ConsensusMetadataService>&
if you don't need to increase the refcount consider passing a raw pointer
http://gerrit.cloudera.org:8080/#/c/7191/2/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:
PS2, Line 289: const
does the const server a purpose here?
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Create ConsensusMetadataManager
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>