Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make collection clones of IMap immutable (#12198) #15013

Merged
merged 2 commits into from May 15, 2019

Conversation

@petrpleshachkov
Copy link
Contributor

petrpleshachkov commented May 10, 2019

Consistently throw UnsupportedOperationException on attempt to update a
collection returned by keySet, entrySet, localKeySet, values and getAll
methods.

The fix may change the behavior of existing customers throwing an
exception instead of ignoring the updates.

Moved InflatableSet to internal package making it for internal use only.

Fixes: #12198

Consistently throw UnsupportedOperationException on attempt to update a
collection returned by keySet, entrySet, localKeySet, values and getAll
methods.

The fix may change the behavior of existing customers throwing an
exception instead of ignoring the updates.

Moved InflatableSet to internal package making it for internal use only.

Fixes: #12198
@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented May 10, 2019

run-lab-run

super(compactList);
}

public static <T> ImmutableBuilder<T> newImmutableBuilder(int initialCapacity) {

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris May 13, 2019

Contributor

rename to newBuilder? The builder itself is not immutable.

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov May 14, 2019

Author Contributor

I already have newBuilder(int) in the superclass. Maybe newImmutableSetBuilder? To avoid confusion I can rename class ImmutableBuider->ImmutableSetBuilder

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris May 14, 2019

Contributor

ah, I see. newImmutableSetBuilder sounds good to me.

}

public ImmutableInflatableSet<T> build() {
ImmutableInflatableSet<T> set = new ImmutableInflatableSet<T>(list);

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris May 13, 2019

Contributor

minor: no need to repeat type argument in constructor

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov May 14, 2019

Author Contributor

The same.

@Override
public Iterator<T> iterator() {
if (state == State.INFLATED) {
return Collections.unmodifiableSet(inflatedSet).iterator();

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris May 13, 2019

Contributor

As far as I can tell, since this immutable set is always constructed via its builder and there are never additions, there should be no case in which the set will be inflated -> this line should not be executed. Maybe throw an illegal state exception instead?

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov May 14, 2019

Author Contributor

Sounds good, I will do this.

Minor changes to remove unreachable code and adjust names of some classes.
@petrpleshachkov petrpleshachkov force-pushed the petrpleshachkov:fix/master/12198 branch from 2196877 to 5930bec May 14, 2019
Copy link
Contributor

vbekiaris left a comment

looks good to me!

@petrpleshachkov petrpleshachkov merged commit 8a1afe4 into hazelcast:master May 15, 2019
1 check passed
1 check passed
default Test PASSed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.