You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/06/08 01:43:17 UTC
[kudu-CR] WIP consensus: remove env usage from ConsensusMetadata
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10649
Change subject: WIP consensus: remove env usage from ConsensusMetadata
......................................................................
WIP consensus: remove env usage from ConsensusMetadata
This patch removes the env usage from ConsensusMetadata in an effort to
decouple the metadata from its storage format. Instead, the
ConsensusMetadataManager manages writing and reading, as it already
manages load and creation of cmeta. This separation will make changes
to the cmeta's on-disk storage possible without changing the interfaces
and implementation of the ConsensusMetadata class.
Some of the semantics are changed for the ConsensusMetadataManager.
Namely, because ConsensusMetadata's Create(), Load(), etc., now depend
on the manager, which maintains a cache for the metadata, the
aforementioned functions will all refer to the same ConsensusMetadata
object.
TODO
- finalize the API; maybe there's a cleaner one
- extend stress test for new functions
Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
6 files changed, 130 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/10649/1
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10649
to look at the new patch set (#2).
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
consensus: separate storage from ConsensusMetadata
This patch removes the underlying storage mechanisms from the
ConsensusMetadata class and moves it into the ConsensusMetadataManager.
Instead, the cmeta manager will now coordinate flushing of the cmeta, as
it already manages their loading and creation. This move will make
changes to the cmeta's on-disk storage possible without changing the
interfaces and implementation of the ConsensusMetadata class itself.
Some of the semantics are changed for the ConsensusMetadataManager.
Namely, because ConsensusMetadata's Create(), Load(), etc., now depend
on the manager, which maintains a cache for the metadata, the
aforementioned functions will all refer to the same ConsensusMetadata
object.
Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
7 files changed, 260 insertions(+), 128 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/10649/2
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10649 )
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
Patch Set 4: Code-Review+1
(3 comments)
Looks pretty good to me. Only thing is I'm not sure how this change really helps create a better abstraction.
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc@265
PS4, Line 265: const string& peer_uuid = fs_manager_.uuid();
is this change required?
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc@341
PS4, Line 341: peer_uuid
nit: missing leading space
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc@347
PS4, Line 347: peer_uuid
space
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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-Comment-Date: Tue, 19 Jun 2018 22:44:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP consensus: remove env usage from ConsensusMetadata
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/10649 )
Change subject: WIP consensus: remove env usage from ConsensusMetadata
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10649/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10649/1//COMMIT_MSG@7
PS1, Line 7: WIP consensus: remove env usage from ConsensusMetadata
this is a bit mistleading, this is more about decoupling ConsensusMetadata from FsManager and pipe (the leftover operation) the operations though ConsensusMetadataManager
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h
File src/kudu/consensus/consensus_meta_manager.h:
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h@52
PS1, Line 52: Status Init() const;
docs
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h@91
PS1, Line 91: void ClearCache(const std::string& tablet_id);
docs
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Jun 2018 21:52:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10649
to look at the new patch set (#3).
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
consensus: separate storage from ConsensusMetadata
This patch removes the underlying storage mechanisms from the
ConsensusMetadata class and moves it into the ConsensusMetadataManager.
Instead, the cmeta manager will now coordinate flushing of the cmeta, as
it already manages their loading and creation. This move will make
changes to the cmeta's on-disk storage possible without changing the
interfaces and implementation of the ConsensusMetadata class itself.
Some of the semantics are changed for the ConsensusMetadataManager.
Namely, because ConsensusMetadata's Create(), Load(), etc., now depend
on the manager, which maintains a cache for the metadata, the
aforementioned functions will all refer to the same ConsensusMetadata
object.
Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
7 files changed, 267 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/10649/3
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10649 )
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10649/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10649/1//COMMIT_MSG@7
PS1, Line 7: consensus: separate storage from ConsensusMetadata
> this is a bit mistleading, this is more about decoupling ConsensusMetadata
Done
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h
File src/kudu/consensus/consensus_meta_manager.h:
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h@52
PS1, Line 52: public:
> docs
Done
http://gerrit.cloudera.org:8080/#/c/10649/1/src/kudu/consensus/consensus_meta_manager.h@91
PS1, Line 91: ConsensusMetadata::FlushMode flus
> docs
Done
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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-Comment-Date: Tue, 19 Jun 2018 18:21:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/10649 )
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
Patch Set 4:
(5 comments)
I guess andrew's point is that if we want to change where/how we store the consensus meta (.e.g if we want to move it to rocksdb or something), with this change we only need to do it in one place, versus having to change consensusmetadata too.
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc@322
PS4, Line 322: const string& peer_uuid = fs_manager_.uuid();
same as mike's comment above. this is a test, I think copying this string (and defending against fs_manager lifecycle bugs) is probably best
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h
File src/kudu/consensus/consensus_meta_manager.h:
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@56
PS4, Line 56: Status Init() const;
hum, Init() is const? that's a bit weird...
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@112
PS4, Line 112: LoadFromDisk
do we need to have the "fromdisk" suffix? I thought the point of this change was that we could store the metadata elsewhere, e.g. rocksdb
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@117
PS4, Line 117: FlushToDisk
same
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.cc
File src/kudu/consensus/consensus_meta_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.cc@153
PS4, Line 153: const string& cmeta_path = fs_manager_->GetConsensusMetadataPath(tablet_id);
: RETURN_NOT_OK_PREPEND(fs_manager_->env()->DeleteFile(cmeta_path),
: Substitute("Unable to delete consensus metadata for tablet $0", tablet_id));
: lock_guard<Mutex> l(lock_);
: cmeta_cache_.erase(tablet_id); // OK to delete uncached cmeta; ignore the return value.
: return Status::OK();
hum, are you sure the change of ordering here doesn't matter?
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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-Comment-Date: Mon, 25 Jun 2018 17:59:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus: separate storage from ConsensusMetadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10649
to look at the new patch set (#4).
Change subject: consensus: separate storage from ConsensusMetadata
......................................................................
consensus: separate storage from ConsensusMetadata
This patch removes the underlying storage mechanisms from the
ConsensusMetadata class and moves it into the ConsensusMetadataManager.
Instead, the cmeta manager will now coordinate flushing of the cmeta, as
it already manages their loading and creation. This move will make
changes to the cmeta's on-disk storage possible without changing the
interfaces and implementation of the ConsensusMetadata class itself.
ConsensusMetadata's Create(), Load(), and DeleteOnDisk() methods have
been removed in favor of the ConsensusMetadataManager equivalents.
Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_meta_manager.cc
M src/kudu/consensus/consensus_meta_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
7 files changed, 267 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/10649/4
--
To view, visit http://gerrit.cloudera.org:8080/10649
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923
Gerrit-Change-Number: 10649
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot