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 <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-HasComments: Yes

Reply via email to