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

Remove and ban ImmutableMap#entrySet #13724

Merged
merged 1 commit into from Sep 23, 2015

Conversation

Projects
None yet
3 participants
@nik9000
Contributor

nik9000 commented Sep 22, 2015

Banning ImmutableSet outright is too much to do all at once - this starts
the process by banning ImmutableMap#entrySet - one of the more common ways
that ImmutableSets come up. It then starts to remove calls to
ImmutableMap#entrySet by changing declarations from ImmutableMap to Map.

Unfortunately this process is like pulling on a long, windy string and one
declaration change requires another which requires 5 more which in turn
require another few. So there are lots of scattered changes to do this.

As such, to keep these commits manageable they only remove ImmutableMap from
the signatures that are needed for entrySet and make no effort to stop using
ImmutableMap internally. Removing the usages of ImmutableMap complicates
immutability guarantees and should be done separately.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 22, 2015

Contributor

@jasontedor, this is the start of the ImmutableSet work.

Contributor

nik9000 commented Sep 22, 2015

@jasontedor, this is the start of the ImmutableSet work.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 22, 2015

Contributor

Note to any reviewers: I feel like I've played a bit fast a loose with immutability guarantees in places. ImmutableMap ensured returns were immutable all through the call stack while Collections.unmodifiableMap is really only enforced at map construction time and this commit feels like it weakens the immutability guarantees somewhat. I suspect that is just the price of dropping guava though.

Contributor

nik9000 commented Sep 22, 2015

Note to any reviewers: I feel like I've played a bit fast a loose with immutability guarantees in places. ImmutableMap ensured returns were immutable all through the call stack while Collections.unmodifiableMap is really only enforced at map construction time and this commit feels like it weakens the immutability guarantees somewhat. I suspect that is just the price of dropping guava though.

@jasontedor jasontedor referenced this pull request Sep 22, 2015

Closed

Remove Guava as a dependency #13224

72 of 72 tasks complete

@jasontedor jasontedor added the review label Sep 23, 2015

@jasontedor jasontedor self-assigned this Sep 23, 2015

@jasontedor

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Sep 23, 2015

Member

@nik9000 Yeah, the change from ImmutableX to unmmodifiableX that you correctly point out is a known loss. In most cases, ImmutableX wasn't being exposed through the API so the immutability guarantees of one over the other should not have been being relied upon. If we do end up needing the stronger immutability guarantees, we will have to revisit.

Member

jasontedor commented Sep 23, 2015

@nik9000 Yeah, the change from ImmutableX to unmmodifiableX that you correctly point out is a known loss. In most cases, ImmutableX wasn't being exposed through the API so the immutability guarantees of one over the other should not have been being relied upon. If we do end up needing the stronger immutability guarantees, we will have to revisit.

@jasontedor

View changes

Show outdated Hide outdated dev-tools/src/main/resources/forbidden/all-signatures.txt
@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Sep 23, 2015

Member

@nik9000 Thank you tackling this one, this looks great. Left a few minor comments about comments, but otherwise LGTM. Push at your leisure. :)

Member

jasontedor commented Sep 23, 2015

@nik9000 Thank you tackling this one, this looks great. Left a few minor comments about comments, but otherwise LGTM. Push at your leisure. :)

@jasontedor jasontedor removed the review label Sep 23, 2015

Remove and ban ImmutableMap#entrySet
Banning `ImmutableSet` outright is too much to do all at once - this starts
the process by banning `ImmutableMap#entrySet` - one of the more common ways
that `ImmutableSet`s come up. It then starts to remove calls to
`ImmutableMap#entrySet` by changing declarations from `ImmutableMap` to `Map`.

Unfortunately this process is like pulling on a long, windy string and one
declaration change requires another which requires 5 more which in turn
require another few. So this change is rather large.

As such, to keep the changes manageable they only remove `ImmutableMap` from
the signatures that are needed for `entrySet` and make little effort to stop
using `ImmutableMap` internally. Removing the usages of `ImmutableMap`
complicates immutability guarantees and will be done separately.
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

Squashed and rebased and caught some more tricksy ImmutableMap#entrySet calls that snuck around my first cull.

Contributor

nik9000 commented Sep 23, 2015

Squashed and rebased and caught some more tricksy ImmutableMap#entrySet calls that snuck around my first cull.

nik9000 added a commit that referenced this pull request Sep 23, 2015

@nik9000 nik9000 merged commit 13720eb into elastic:master Sep 23, 2015

1 check passed

CLA Commit author has signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment