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.

Sign up for GitHub

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

Jump to bottom

Add support of getting a Java stream on ImmutableOpenMap #76921

Merged
merged 25 commits into from
Sep 8, 2021
Merged

Add support of getting a Java stream on ImmutableOpenMap #76921

merged 25 commits into from
Sep 8, 2021

Conversation

arteam
Copy link
Contributor

@arteam arteam commented Aug 25, 2021 •
edited
Loading

ImmutableOpenMap doesn't support Java streams directly, so consumers have to invoke StreamSupport.spliterator if they want to get a Java stream on it. The stream is actually not safe because ObjectObjectCursor is mutable. E.g.

This works:

stream().map(c -> c.value.something()).collect()

This doesn't work

stream().sorted((Comparator.comparing(a.key)).collect()

We can simplify consumer code by providing a direct safe stream method in ImmutableOpenMap which would iterate over immutable map entries. We can achieve that by creating own spliterator over the native ObjectObjectHashMap iterator which copies the mutable cursor to an immutable Map.Entry, which in turn should makes the stream safe.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@arteam arteam force-pushed the add-stream-to-immutable-openmap branch 3 times, most recently from 23b11fc to 10a8154 Compare August 25, 2021 13:03
@elastic elastic deleted a comment from pirateskipper Aug 25, 2021
@elastic elastic deleted a comment from pirateskipper Aug 25, 2021
@arteam
Copy link
Contributor Author

arteam commented Aug 25, 2021

@elasticmachine
run elasticsearch-ci/part-2

@arteam
Copy link
Contributor Author

arteam commented Aug 25, 2021

@elasticmachine
run elasticsearch-ci/bwc
run elasticsearch-ci/rest-compatibility

@arteam
Add support of getting a Java stream on ImmutableOpenMap
a68f350 
`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`
@arteam arteam force-pushed the add-stream-to-immutable-openmap branch from 10a8154 to a68f350 Compare August 25, 2021 17:46
@arteam arteam added auto-backport Automatically create backport pull requests when merged v7.16.0 labels Aug 25, 2021
@arteam
Copy link
Contributor Author

arteam commented Aug 30, 2021

@elasticmachine update branch

henningandersen
henningandersen requested changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Artem, this is a bit of an annoying corner. I left a suggestion.

server/src/main/java/org/elasticsearch/common/collect/ImmutableOpenMap.java Outdated Show resolved Hide resolved
@arteam arteam force-pushed the add-stream-to-immutable-openmap branch from 71d7b98 to 490a13c Compare September 2, 2021 15:39
henningandersen
henningandersen reviewed Sep 3, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach taken has some unnecessary costs, I would like to see if that can be addressed?

server/src/main/java/org/elasticsearch/common/collect/ImmutableOpenMap.java Outdated Show resolved Hide resolved
@arteam arteam force-pushed the add-stream-to-immutable-openmap branch from 376fb0c to 195d207 Compare September 3, 2021 08:54
henningandersen
henningandersen approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

server/src/test/java/org/elasticsearch/common/collect/ImmutableOpenMapTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/collect/ImmutableOpenMapTests.java Outdated Show resolved Hide resolved
@arteam
Copy link
Contributor Author

arteam commented Sep 8, 2021

@elasticmachine update branch

@arteam arteam merged commit 411d7e7 into elastic:master Sep 8, 2021
@arteam arteam deleted the add-stream-to-immutable-openmap branch September 8, 2021 15:27
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 7.x Commit could not be cherrypicked due to conflicts

To backport manually run backport --upstream elastic/elasticsearch --pr 76921

@arteam arteam mentioned this pull request Sep 8, 2021
Closed
arteam added a commit to arteam/elasticsearch that referenced this pull request Sep 8, 2021
@arteam
[7.x] Add support of getting a Java stream on ImmutableOpenMap (elast…
cbcbf33 
…ic#76921)

`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`
arteam added a commit to arteam/elasticsearch that referenced this pull request Sep 8, 2021
@arteam
[7.x] Add support of getting a Java stream on ImmutableOpenMap (elast…
5dec01b 
…ic#76921)

Backports elastic#76921 to 7.x

`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`
@arteam arteam mentioned this pull request Sep 8, 2021
Merged
arteam added a commit to arteam/elasticsearch that referenced this pull request Sep 8, 2021
@arteam
[7.x] Add support of getting a Java stream on ImmutableOpenMap (elast…
1f2ec4d 
…ic#76921)

`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`
arteam added a commit that referenced this pull request Sep 9, 2021
@arteam
[7.x] Add support of getting a Java stream on ImmutableOpenMap (#76921)…
fd0370d 
… (#77455)

* [7.x] Add support of getting a Java stream on ImmutableOpenMap (#76921)

Backports #76921 to 7.x

`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`

* [7.x] Add support of getting a Java stream on ImmutableOpenMap (#76921)

`ImmutableOpenMap` doesn't support Java streams directly, so consumer have
to invoke `StreamSupport.spliterator` on it if they want to run a Java
stream over it. We can simplify consumer code by providing a direct
`stream` method in `ImmutableOpenMap`
@arteam arteam mentioned this pull request Sep 23, 2021
Merged
@arteam arteam mentioned this pull request Oct 4, 2021
Merged
@tlrx tlrx mentioned this pull request Oct 4, 2021
Merged
arteam added a commit that referenced this pull request Oct 5, 2021
@arteam
Expose ImmutableOpenMap.values as a Java Collection (#78529)
cb73e3f 
#76921 added support for getting entries as a Java stream, #77897 added the ability to get keys as a Set. This PR adds the values method which returns a Collection to bring the ImmutableOpenMap API closer to java.util.Map
@arteam arteam mentioned this pull request Oct 5, 2021
Merged
arteam added a commit to arteam/elasticsearch that referenced this pull request Oct 5, 2021
@arteam
Expose ImmutableOpenMap.values as a Java Collection (elastic#78529)
9b6f449 
elastic#76921 added support for getting entries as a Java stream, elastic#77897 added the ability to get keys as a Set. This PR adds the values method which returns a Collection to bring the ImmutableOpenMap API closer to java.util.Map

(cherry picked from commit cb73e3f)
arteam added a commit that referenced this pull request Oct 5, 2021
@arteam
[7.x] Expose ImmutableOpenMap.values as a Java Collection (#78529) (#…
96fe440 
…78709)

#76921 added support for getting entries as a Java stream, #77897 added the ability to get keys as a Set. This PR adds the values method which returns a Collection to bring the ImmutableOpenMap API closer to java.util.Map

(cherry picked from commit cb73e3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henningandersen henningandersen henningandersen approved these changes

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >non-issue >refactoring Team:Core/Infra Meta label for core/infra team v7.16.0 v8.0.0-alpha2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arteam @elasticmachine @elasticsearchmachine @henningandersen @jakelandis