[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Add ComputeIfAbsent methods to map-util
..


Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a pointer 
to the
value. The pair includes the pointer to the value, but also a bool indicating 
whether
the value was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Reviewed-on: http://gerrit.cloudera.org:8080/3593
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 70 insertions(+), 0 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 9: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/8/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 742: const MvccSnapshot& 
snRollingDiskRowSetWriter* out) {
> err?
oops. Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/2303/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a pointer 
to the
value. The pair includes the pointer to the value, but also a bool indicating 
whether
the value was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 70 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/8/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 742: const MvccSnapshot& 
snRollingDiskRowSetWriter* out) {
err?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 8:

Build Started http://104.196.14.100/job/kudu-gerrit/2301/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a pointer 
to the
value. The pair includes the pointer to the value, but also a bool indicating 
whether
the value was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/tablet/compaction.cc
M src/kudu/util/map-util-test.cc
3 files changed, 71 insertions(+), 2 deletions(-)


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3593/7/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 65: #include 
> Is it really needed here?
Done


PS7, Line 800: CHECK
> this seems to have turned back to a CHECK instead of DCHECK
Done


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/7/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

PS7, Line 800: CHECK
this seems to have turned back to a CHECK instead of DCHECK


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 7: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/7/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 65: #include 
Is it really needed here?


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 7:

Build Started http://104.196.14.100/job/kudu-gerrit/2244/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a pointer 
to the
value. The pair includes the pointer to the value, but also a bool indicating 
whether
the value was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 71 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/3593/7
-- 
To view, visit http://gerrit.cloudera.org:8080/3593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 792: pair
Why not just MapContainer::mapped_type, but 
MapContainer::value_type::first_type?


Line 793: ComputeIfAbsentReturnAbsense(MapContainer* container,
> it's due to code style. we pass by pointer when we'll be mutating the conta
OK, code style is code style, whether it's ugly or not.

But if using pointers, why there is no check for a nullptr then?  Is this ok to 
get SIGSEGV when calling this function with a null pointer as a parameter or 
there should be some code which checks for null before de-referencing it?


Line 794:  const typename 
MapContainer::value_type::first_type& key,
MapContainer::value_type::first_type --> MapContainer::key_type?


Line 814: typename MapContainer::value_type::second_type* const
MapContainer::mapped_type?


Line 815: ComputeIfAbsent(MapContainer* container,
Does in make sense to reflect the fact the value is being inserted?  I.e., name 
this InsertComputedIfAbsent()


Line 816: const typename MapContainer::value_type::first_type& 
key,
MapContainer::value_type::first_type --> MapContainer::key_type?


http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

Line 58: TEST(ComputeIfAbsentTest, TestComputeIfAbsentAntReturnAbsense) {
typo? TestComputeIfAbsentAntReturnAbsense --> 
TestComputeIfAbsentAndReturnAbsense

Or just rename it into TestComputeIfAbsentReturnAbsense


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 780: // it uses the lambda function to create a value, inserts it into the 
map, and returns a a pair with
typo: an extra 'a': '...and returns a a pair...'


Line 791: template 
How costly is compute_func compared with locating the key in the map?  If in 
most use cases compute_func is cheap, then it makes sense just to use stock 
insert() method right away, not trying to locate the element first and not 
calling this method.  But I suppose this is not the case, right?


Line 793: ComputeIfAbsentReturnAbsense(MapContainer* container,
Why not to pass the container by reference here?  Is that due to code style?  
If passing by pointer, what if parameter is null?  Is any check needed here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2231/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a pointer 
to the
value. The pair includes the pointer to the value, but also a bool indicating 
whether
the value was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 71 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/3593/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/4/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 814: const typename MapContainer::value_type::second_type&
> I won't be surprised if you need a non-const reference return value instead
yeah, tsan didn't like it. change this to return typename 
MapContainer::value_type::second_type* const like other methods above


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/4/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 814: const typename MapContainer::value_type::second_type&
I won't be surprised if you need a non-const reference return value instead of 
a const one when you try to use this, but guess we will find out :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2230/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3593/3/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

PS3, Line 779: it 
> exist
Done


PS3, Line 791: std::function 
> rather than using std::function I think you can templatize on a function ty
Done


PS3, Line 795: CH
> I think a DCHECK would be better here, since we probably want this function
Done


Line 806: ComputeIfAbsentReturnAbsense(MapContainer* container,
> could you implement ComputeIfAbsent as just calling ComputeIfAbsentReturnAb
Done


http://gerrit.cloudera.org:8080/#/c/3593/3/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

PS3, Line 54:  {
> nit: inconsistent spacing (also below)
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3593/3/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

PS3, Line 779: it 
exist


PS3, Line 791: std::function 
rather than using std::function I think you can templatize on a function type 
here. http://stackoverflow.com/questions/14677997/stdfunction-vs-template seems 
to agree this is usually preferred (the optimizer isn't always smart enough to 
see the equivalence)


PS3, Line 795: CH
I think a DCHECK would be better here, since we probably want this function to 
be as small as possible to be inlined


Line 806: ComputeIfAbsentReturnAbsense(MapContainer* container,
could you implement ComputeIfAbsent as just calling 
ComputeIfAbsentReturnAbsense?


http://gerrit.cloudera.org:8080/#/c/3593/3/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

PS3, Line 54:  {
nit: inconsistent spacing (also below)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a reference 
to the
value. The pair includes the reference, but also a bool indicating whether the 
value
was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 69 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/3593/3
-- 
To view, visit http://gerrit.cloudera.org:8080/3593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2229/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2228/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a reference 
to the
value. The pair includes the reference, but also a bool indicating whether the 
value
was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 67 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/3593/2
-- 
To view, visit http://gerrit.cloudera.org:8080/3593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add ComputeIfAbsent methods to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Add ComputeIfAbsent methods to map-util
..

Add ComputeIfAbsent methods to map-util

This adds two new ComputeIfAbsent methods to map-util, inspired by java's 
implementation,
which can be found here:

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-

The first one, ComputeIfAbsent, is a simple c++ implementation of the exact same
concept: Retreive a value from a map; if it doesn't exist compute it using a 
lambda
function and insert it first.

The second one is a slight variation that returns a pair instead of a reference 
to the
value. The pair includes the reference, but also a bool indicating whether the 
value
was initially absent.

Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 67 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/3593/1
-- 
To view, visit http://gerrit.cloudera.org:8080/3593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves