You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/09/18 00:09:09 UTC
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Hello Dan Burkert, Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8089
to review the following change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/cow_object.h
6 files changed, 245 insertions(+), 198 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/1
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc@3703
PS7, Line 3703: lock_out.AddMutableInfos({ std::move(new_tablet) });
should this just use AddMutableInfo now instead of the vector?
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@353
PS7, Line 353: // 2. That if the CowGroupLock is already locked in a particular mode,
is it possible to have a DCHECK for this in some way? it seems like an API that is a bit subtle and easy to misuse with potentially confusing results
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@355
PS7, Line 355: const Key& key
perhaps take this by value so you can emplace(std::move(key), object) into the map and avoid a copy?
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@356
PS7, Line 356: cows_[key] = const_cast<CowObject<Value>*>(object);
is it worth a CHECK that if we are replacing an existing entry that the 'object' matches?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Sep 2017 05:00:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#5).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 397 insertions(+), 304 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/5
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#7).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 399 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/7
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#6).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 399 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/6
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> Why is it necessary to have a key for every object? Would set of pointers
Pointers would indeed provide a deterministic ordering, but they don't let the CowGroupLock user dictate exactly what that ordering should be. By forcing users to provide the key, the catalog manager can enforce that the deterministic ordering is e.g. tablet ID lexicographic order.
PS2, Line 240: A
: // key must not be modified while its corresponding CowObject is held by the
: // lock
> I don't understand this bit. The API statically guarantees that they key d
I was trying to explain that it would be weird if the key changed, since it's external to the value. But that's kind of a strange thing to say, and it's inherently true of an std::map, so I'll drop it.
Line 259: // CowGroupLock<string, Foo> l(CowGroupLock<string, Foo>::RELEASED);
> The CowGroupLock<string, Foo> thing is really ugly. C++ won't let you leav
I'll convert LockMode into an enum class and reuse it in both CowLock and CowGroupLock.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 6: Verified+1
Overriding Jenkins, unrelated test failures.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 352: cows_[key] = const_cast<CowObject<Value>*>(object);
> Is it worth adding a note about that? From the doc for this method I read
Sure.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc@2518
PS7, Line 2518: *modified_report.mutable_consensus_state() = cstate;
> Consider using std::move(cstate), although if you think it's too likely to
I'll punt since it's unrelated and will be replaced in the next patch anyway.
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc@3703
PS7, Line 3703: lock_out.AddMutableInfos({ std::move(new_tablet) });
> should this just use AddMutableInfo now instead of the vector?
Done
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@353
PS7, Line 353: // 2. That if the CowGroupLock is already locked in a particular mode,
> is it possible to have a DCHECK for this in some way? it seems like an API
I think we'll already get DCHECKs from within CowObject/RWCLock during Commit/Unlock, but I don't mind adding explicit DCHECKs here too.
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@355
PS7, Line 355: const Key& key
> perhaps take this by value so you can emplace(std::move(key), object) into
It won't have any effect for TableInfo/TabletInfo (since id() returns a cref), but sure.
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/util/cow_object.h@356
PS7, Line 356: cows_[key] = const_cast<CowObject<Value>*>(object);
> is it worth a CHECK that if we are replacing an existing entry that the 'ob
So comparing pointers? Yeah, I suppose so. I can't think of a use case for allowing them to actually differ.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Sep 2017 22:24:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> Not sure I follow. If this map were an array, the user would have to supply
I thought it would be natural to add COW object pointers in the desired order.
Maybe, it makes sense to introduce a constructor that accepts a vector of such objects, so it would be possible to write just
vector<CowObject<Foo>> foos;
...
CowGroupLock<Foo> l(RELEASED, foos);
PS2, Line 261: c
typo? Should it be 'l' instead?
PS2, Line 273: c
'l' ?
PS2, Line 282: c
'l' ?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 5: Verified+1
Overriding Jenkins, failure was KUDU-2059.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 8: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8089/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8089/8//COMMIT_MSG@13
PS8, Line 13: Importantly, the default out-of-scope behavior is now Unlock()
+1
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h@328
PS8, Line 328: break;
nit: just to be in sync with the code at line 397, consider moving this to a separate line.
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h@375
PS8, Line 375: DCHECK_EQ(r.first->second, object);
Nice. That's what I also asked to add in my review for PS2.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Sep 2017 18:33:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2: Verified+1
Overriding Jenkins, the only failure was an instance of KUDU-1521.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#2).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/cow_object.h
6 files changed, 251 insertions(+), 200 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/2
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
Why is it necessary to have a key for every object? Would set of pointers suffice?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8089/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS5, Line 3689: before
> after?
Whoops, yes.
http://gerrit.cloudera.org:8080/#/c/8089/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
PS5, Line 353:
> Style nit: if I'm not mistaken, it should be 4 spaces here instead of 2.
I was just copying what MetadataLock did. I'll fix both.
PS5, Line 357: this->
> Is it necessary?
If I don't use it, I get:
../../src/kudu/master/catalog_manager.h: In instantiation of ‘void kudu::master::MetadataGroupLock<MetadataClass>::AddMutableInfo(MetadataClass*) [with MetadataClass = kudu::master::TabletInfo]’:
../../src/kudu/master/catalog_manager.cc:3187:45: required from here
../../src/kudu/master/catalog_manager.h:355:21: error: ‘AddMutableObject’ was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
AddMutableObject(info->id(), info->mutable_metadata());
^
../../src/kudu/master/catalog_manager.h:355:21: note: declarations in dependent base ‘kudu::CowGroupLock<std::__cxx11::basic_string<char>, kudu::master::PersistentTabletInfo>’ are not found by unqualified lookup
../../src/kudu/master/catalog_manager.h:355:21: note: use ‘this->AddMutableObject’ instead
PS5, Line 361: this->
> Is this necessary?
See above.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> I thought it would be natural to add COW object pointers in the desired ord
From the other side, it seems in this particular case for catalog manager the it's more natural to rely on tablet id as a key.
Probably, your original approach is better in this context then.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: A
: // key must not be modified while its corresponding CowObject is held by the
: // lock
I don't understand this bit. The API statically guarantees that they key does not change, so why warn against it?
Line 259: // CowGroupLock<string, Foo> l(CowGroupLock<string, Foo>::RELEASED);
The CowGroupLock<string, Foo> thing is really ugly. C++ won't let you leave of the template params? If not, consider making this a proper enum class outside of CowGroupLock.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 352: cows_[key] = const_cast<CowObject<Value>*>(object);
> Does it make sense to add a debug assertion to make sure we don't replace a
That's actually a "feature", not a "bug". For example, when locking tables, the code I wrote iterates over a group of tablets and adds each one's table into the lock, even though several tablets may share the same table.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#4).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 396 insertions(+), 304 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/4
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 352: cows_[key] = const_cast<CowObject<Value>*>(object);
> That's actually a "feature", not a "bug". For example, when locking tables,
Is it worth adding a note about that? From the doc for this method I read 'Add a new CowObject ...', so I thought it should always be some new pair added, not replaced, into the map.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 29 Sep 2017 17:55:53 +0000
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#8).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 436 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/8
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Dan Burkert, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8089
to look at the new patch set (#9).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 439 insertions(+), 306 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/9
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#3).
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 396 insertions(+), 304 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8089/3
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 8: Verified+1
Overriding Jenkins, the latest run hit two flaky tests.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 23 Sep 2017 00:13:58 +0000
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> Ah, that makes sense.
Not sure I follow. If this map were an array, the user would have to supply a comparator (so that Lock() could sort by e.g. tablet IDs), or would have to insert in sorted order. How are either of those better than inserting a key with the value?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
catalog_manager: make ScopedTabletInfoCommitter generic
I want to use it for TableInfo mutations, and to manage multiple read locks
too. So I've moved it to util/cow_object, created a read-only variant, and
templatized them for any CowObject.
Importantly, the default out-of-scope behavior is now Unlock(). Previously it
was Commit(), but I think Unlock() is less surprising.
Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Reviewed-on: http://gerrit.cloudera.org:8080/8089
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/cow_object.cc
M src/kudu/util/cow_object.h
10 files changed, 439 insertions(+), 306 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 5: Code-Review+1
(4 comments)
Spotted a few nits (sorry -- I didn't see them in prior passes). Otherwise LGTM.
http://gerrit.cloudera.org:8080/#/c/8089/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS5, Line 3689: before
after?
http://gerrit.cloudera.org:8080/#/c/8089/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
PS5, Line 353:
Style nit: if I'm not mistaken, it should be 4 spaces here instead of 2.
PS5, Line 357: this->
Is it necessary?
PS5, Line 361: this->
Is this necessary?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> Pointers would indeed provide a deterministic ordering, but they don't let
Ah, that makes sense.
For some reason it seems as an extra parameter to me. Would it make sense to use an array internally for that? Given the fact the number of locks would not be greater than a few, that would not make much impact on performance.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 352: cows_[key] = const_cast<CowObject<Value>*>(object);
Does it make sense to add a debug assertion to make sure we don't replace already existing {key, value} pair with another one? At least, I would expect this to explode if some different object is put under the same key.
PS2, Line 357: cows_[key] = object;
ditto
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h@328
PS8, Line 328: break;
> nit: just to be in sync with the code at line 397, consider moving this to
Done
http://gerrit.cloudera.org:8080/#/c/8089/8/src/kudu/util/cow_object.h@375
PS8, Line 375: DCHECK_EQ(r.first->second, object);
> Nice. That's what I also asked to add in my review for PS2.
Yes, now that I'm rereading what you wrote, I see that I missed that part of your suggestion. Sorry about that.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Sep 2017 18:37:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:57:51 +0000
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8089/7/src/kudu/master/catalog_manager.cc@2518
PS7, Line 2518: *modified_report.mutable_consensus_state() = cstate;
Consider using std::move(cstate), although if you think it's too likely to break in subsequent revisions I'd be alright skipping it.
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h@259
PS2, Line 259: //
> I'll convert LockMode into an enum class and reuse it in both CowLock and C
Thanks, I think this is a big improvement.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:57:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8089/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
PS5, Line 357: this->
> If I don't use it, I get:
Heh, that's interesting. OK, using the suggestion from the compiler sounds good to me. Thank you for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8089 )
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 9: Code-Review+2
LGTM. Perhaps, you would like to hear from Todd as well?
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-Change-Number: 8089
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Sep 2017 18:52:12 +0000
Gerrit-HasComments: No
[kudu-CR] catalog manager: make ScopedTabletInfoCommitter generic
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: make ScopedTabletInfoCommitter generic
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8089/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
PS2, Line 240: The use of std::map forces callers to provide a key for each CowObject
> I thought it would be natural to add COW object pointers in the desired ord
But that erodes one of the values of this class: users don't need to worry about inserting objects in the right order, or inserting all at once; they just need to supply a key when inserting the object.
PS2, Line 261: c
> typo? Should it be 'l' instead?
Done
PS2, Line 273: c
> 'l' ?
Done
PS2, Line 282: c
> 'l' ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/8089
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I890db0fd18f773ab2253c49817def8162e04ad25
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes