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

Removes and bans ImmutableSet #13754

Merged
merged 13 commits into from Sep 26, 2015

Conversation

Projects
None yet
3 participants
@nik9000
Contributor

nik9000 commented Sep 23, 2015

The next step in #13224.

@jasontedor jasontedor referenced this pull request Sep 23, 2015

Closed

Remove Guava as a dependency #13224

72 of 72 tasks complete
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

Note to any reviewers: be very very careful. I made a one line mistake and broke parent/child for a while during this process. The tests all pass but I'm a bit afraid.

Also, some of these changes might change the memory usage of the cluster state. ImmutableSet uses open addressing and a single array but this change switches to HashSets which resolves collisions with trees - so more memory usage.

Contributor

nik9000 commented Sep 23, 2015

Note to any reviewers: be very very careful. I made a one line mistake and broke parent/child for a while during this process. The tests all pass but I'm a bit afraid.

Also, some of these changes might change the memory usage of the cluster state. ImmutableSet uses open addressing and a single array but this change switches to HashSets which resolves collisions with trees - so more memory usage.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

In other odd news, this pr shaves a minute off of a clean compile and test of the core module. I'm not sure why.

Contributor

nik9000 commented Sep 23, 2015

In other odd news, this pr shaves a minute off of a clean compile and test of the core module. I'm not sure why.

@@ -729,16 +743,16 @@ public static Builder builder(MetaData metaData) {
}
/** All known byte-sized cluster settings. */
public static final Set<String> CLUSTER_BYTES_SIZE_SETTINGS = ImmutableSet.of(
public static final Set<String> CLUSTER_BYTES_SIZE_SETTINGS = unmodifiableSet(newHashSet(

This comment has been minimized.

@jpountz

jpountz Sep 24, 2015

Contributor

This can be left as a TODO for another PR but I don't see using Sets.newHashSet as an improvement, when we can directly use the JDK APIs and do new HashSet<>(Arrays.asList(...))

@jpountz

jpountz Sep 24, 2015

Contributor

This can be left as a TODO for another PR but I don't see using Sets.newHashSet as an improvement, when we can directly use the JDK APIs and do new HashSet<>(Arrays.asList(...))

This comment has been minimized.

@nik9000

nik9000 Sep 24, 2015

Contributor

I suppose the difference here is that its our Sets.newHashSet and not Guava's. I don't have a strong opinion here other than that we shouldn't remove that helper method in this PR. If we want it gone it can go in another one.

@nik9000

nik9000 Sep 24, 2015

Contributor

I suppose the difference here is that its our Sets.newHashSet and not Guava's. I don't have a strong opinion here other than that we shouldn't remove that helper method in this PR. If we want it gone it can go in another one.

This comment has been minimized.

@jpountz

jpountz Sep 24, 2015

Contributor

we shouldn't remove that helper method in this PR

ohhh yeaaah, that's fore sure!

@jpountz

jpountz Sep 24, 2015

Contributor

we shouldn't remove that helper method in this PR

ohhh yeaaah, that's fore sure!

This comment has been minimized.

@nik9000

nik9000 Sep 24, 2015

Contributor

I know this is kind of a burried place to have the discussion, but what is wrong with newHashSet as a utility? That people will use it and not see the implicit array creation?

@nik9000

nik9000 Sep 24, 2015

Contributor

I know this is kind of a burried place to have the discussion, but what is wrong with newHashSet as a utility? That people will use it and not see the implicit array creation?

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Sep 24, 2015

Contributor

It looks good to me overall. I think the fact that the PR both moves to the stream API and forbids Immutable makes the review a bit harder, for future refactorings maybe we should try to do it in different pull requests. As far as I am concerned, I think I would find the use of the stream API a bit more readable if the chaining was performed on a separate line for every operation.

Contributor

jpountz commented Sep 24, 2015

It looks good to me overall. I think the fact that the PR both moves to the stream API and forbids Immutable makes the review a bit harder, for future refactorings maybe we should try to do it in different pull requests. As far as I am concerned, I think I would find the use of the stream API a bit more readable if the chaining was performed on a separate line for every operation.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 24, 2015

Contributor

I think the fact that the PR both moves to the stream API and forbids Immutable makes the review a bit harder, for future refactorings maybe we should try to do it in different pull requests.

I imagine we wouldn't really move to Stream at all if we did that - which is ok I guess.

I think I would find the use of the stream API a bit more readable if the chaining was performed on a separate line for every operation.

I've done that a few places. I also reverted back to imperative style in a few places as well.

Contributor

nik9000 commented Sep 24, 2015

I think the fact that the PR both moves to the stream API and forbids Immutable makes the review a bit harder, for future refactorings maybe we should try to do it in different pull requests.

I imagine we wouldn't really move to Stream at all if we did that - which is ok I guess.

I think I would find the use of the stream API a bit more readable if the chaining was performed on a separate line for every operation.

I've done that a few places. I also reverted back to imperative style in a few places as well.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 24, 2015

Contributor

@jpountz I believe this is ready for round two at your leisure. Thanks for reviewing it!

Contributor

nik9000 commented Sep 24, 2015

@jpountz I believe this is ready for round two at your leisure. Thanks for reviewing it!

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 24, 2015

Contributor

Sorry about that! I had unpushed local changes that had addressed some of the formatting.

Contributor

nik9000 commented Sep 24, 2015

Sorry about that! I had unpushed local changes that had addressed some of the formatting.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Sep 24, 2015

Contributor

LGTM

Contributor

jpountz commented Sep 24, 2015

LGTM

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 25, 2015

Contributor

I've merged master into this to pick up the query refactoring changes that were just merged.

Contributor

nik9000 commented Sep 25, 2015

I've merged master into this to pick up the query refactoring changes that were just merged.

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

@nik9000 nik9000 merged commit 2f5b6ea into elastic:master Sep 26, 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