Hazelcast: Implement/override default methods in Java8 collections

[mdogan]

Such as Map.computeIfAbsent(..) or Map.forEach(..) etc.

Related https://github.com/hazelcast/hazelcast/issues/11958

[webashutosh]

@mmedenjak can I take this one up? Happy to take up #11958 along with it (or before it if that helps) ...
Happy to take-up any other issue instead of these ones too.
Thanks!

[Holmistr]

Hi @webashutosh , absolutely you can take it :) I'll assign it to you. @mmedenjak will later provide you some initial pointers to help you get started.

[mmedenjak]

Hi @webashutosh ! Glad you're going to take this one. Let me give you some pointers. I think the two issues are related, the other one just being a subset of the methods whilst this one requests that all JDK8 collection methods are implemented.

Generally speaking, I think there are several options that you can take when going through each individual method:
• ideally, the method should work properly without needing any change on Hazelcast code. I guess some collections will fall under this category and there, you don't need to do anything
• some methods might be covered by using existing Hazelcast features. For instance, you might be able to fix some default java.util.Map methods by using EntryProcessors in the background. That will really help you to fix these methods without introducing too much code.
• unfortunately, some default methods may require you to introduce completely new operations. In this case, you're going to have to go through lots of layers of ceremony. Nothing too tricky, though. It will also require changing the https://github.com/hazelcast/hazelcast-client-protocol so that clients can use it as well. In this case, it's best if you look how an existing implementation does it and follow suit.

Also, make sure any new code is covered by tests, both for the member and the client-side. You can also look at existing tests and reuse them. So, good luck and we'll take it further once you open a new PR.

[webashutosh]

Thanks @Holmistr @mmedenjak, especially for the detailed guidelines! :) I will go through the code and send PRs sometime soon (I'm thinking multiple PRs, few methods at a time)...

[webashutosh]

@mmedenjak @Holmistr starting on this one now that the computeIfPresent() implementation is done ... thanks!

[mmedenjak]

Great, I haven't been able to see if there are any tricky parts left so you'll have to assess the methods one-by-one. You already know how our request-response operations now work so that's a plus.

Ideally, the rest of the methods will either be simple or there will be nothing to change. We'll see. Good luck and ping us with any questions.

[webashutosh]

Working on compute() and merge() implementation

[mmedenjak]

Great, thank you @webashutosh !

[webashutosh]

Just an update - I've been busy with some other work for a few days. Looking forward to continue contributing very soon.

[webashutosh]

Coding in progress. Will be raising a PR by this weekend.

[mmedenjak]

Moving back to 4.1 since I believe that most (if not all) methods have now been covered and we just need to double-check and possibly close this issue.

[Holmistr]

@webashutosh Would you actually mind helping us out with that and double-check that the methods are all done? :)

[webashutosh]

Sure @Holmistr! I had done one round of that already, let me go through them one more time and provide a summary here.

[webashutosh]

@Holmistr @mmedenjak - These are the only Map/ConcurrentMap methods that are Not yet overriden in Hz -
1) getOrDefault(Object key, V defaultValue) - Inherited implementation seems good enough
2) replaceAll(BiFunction<? super K, ? super V, ? extends V> function) - please let me know if we should implement this one?

[mmedenjak]

@webashutosh thank you for verifying! Yes, I think we might override replaceAll for the member-side. Overall, it looks more or less like the forEach method and I think the member-side proxy (MapProxyImpl) can resort to using an entry processor instead of fetching all entries locally. It almost succeeds in doing so, but unfortunately it wraps the BiConsumer instance passed to replaceAll (which might be serializable and static) in another lambda which is definitely not serializable, thus resorting to the worst case when the entire map is fetched locally.

I think we might also improve how forEach works - instead of getting all entries locally using entrySet(), we can iterate through the entries using internal methods such as ClientMapProxy#iterator and MapProxyImpl#iterator. But let's leave that for another time.

Can you send a PR in a couple of days? We're close to finishing 4.1 and it'd be great if we could include the improvement for replaceAll as well.

[webashutosh]

Sure @mmedenjak, will do!