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

Use Java 11 collections conveniences everywhere #41399

Merged
merged 21 commits into from
Apr 26, 2019
Merged

Use Java 11 collections conveniences everywhere #41399

merged 21 commits into from
Apr 26, 2019

Conversation

jasontedor
Copy link
Member

This commit replaces all applicable uses with Java 11 collections convenience methods.

Relates #41374

@jasontedor
Use Java 11 collections conveniences everywhere
be632fb 
This commit replaces all applicable uses with Java 11 collections
convenience methods.
@jasontedor
Iterator
cdd45e8
@jasontedor jasontedor added >non-issue :Core/Infra/Core Core issues without another label v8.0.0 labels Apr 21, 2019
@jasontedor jasontedor requested a review from jpountz April 21, 2019 17:15
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

rjernst
rjernst reviewed Apr 22, 2019
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This looks good, I just have the one real comment about why set is changed to list in a couple places.

client/rest-high-level/src/main/java/org/elasticsearch/client/security/GetUsersResponse.java Outdated Show resolved Hide resolved
client/rest-high-level/src/main/java/org/elasticsearch/client/security/user/User.java Outdated Show resolved Hide resolved
...in/java/org/elasticsearch/client/security/user/privileges/ApplicationResourcePrivileges.java Show resolved Hide resolved
rjernst
rjernst approved these changes Apr 23, 2019
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. LGTM.

jpountz
jpountz approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left some minor comments about indentation but it LGTM. I wish Intellij didn't feel the need to reorganize imports in every file it touches. :)

server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java Outdated
o.writeByte((byte) 3);
o.writeFloat((float) v);
}),
entry(Double.class,
Copy link
Contributor

Choose a reason for hiding this comment

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

not indented like other entries

server/src/main/java/org/elasticsearch/index/mapper/DocumentMapperParser.java
* - this should almost surely be a copy as a LinkedHashMap to have the ordering guarantees that we are relying on
* - investigate the above note about whether or not we really need to be copying here, the ideal outcome would be to not
*/
docBuilder.meta(Collections.unmodifiableMap(new HashMap<>(meta)));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you open an issue about it? It feels like it could be a source of trouble if a user were to switch to a different JVM (either different vendor or newer version)?

server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java Outdated
LAMBDAS = unmodifiableMap(lamdas);
DISTRIBUTIONS = Map.of("ll", new DistributionLL(), "spl", new DistributionSPL());

LAMBDAS = Map.of("df", new LambdaDF(), "ttf", new LambdaTTF());
Copy link
Contributor

Choose a reason for hiding this comment

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

move initialization to the declaration and remove the static block, like you did for other constants?

server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java
ScriptSortBuilder.NAME, ScriptSortBuilder::fromXContent,
GeoDistanceSortBuilder.NAME, GeoDistanceSortBuilder::fromXContent,
GeoDistanceSortBuilder.ALTERNATIVE_NAME, GeoDistanceSortBuilder::fromXContent,
// TODO: this can deadlock as it might access the ScoreSortBuilder (subclass) initializer from the SortBuilder initializer!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks serious enough to open an issue?

server/src/main/java/org/elasticsearch/transport/TransportService.java Outdated
)));
public static final Set<String> VALID_ACTION_PREFIXES = Set.of(
"indices:admin",
"indices:monitor", "indices:data/write",
Copy link
Contributor

Choose a reason for hiding this comment

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

new line?

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java Outdated
@@ -3,16 +3,19 @@
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* the Apache License,
Version 2.0 (the "License"); you may
Copy link
Contributor

Choose a reason for hiding this comment

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

something weird happened here

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java Outdated
* "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND,
either express or implied. See the License for the
Copy link
Contributor

Choose a reason for hiding this comment

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

and there

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java Outdated
@@ -182,7 +179,8 @@ public void cleanUp() throws Exception {
}

public void testAllAggsAreBeingTested() {
assertEquals(InternalAggregationTestCase.getDefaultNamedXContents().size(), aggsTests.size());
assertEquals(InternalAggregationTestCase.getDefaultNamedXContents().size(),
aggsTests.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation issue, there are more of them below as well

@jasontedor
Fix tests
96dfcea
@jasontedor
Reliable ordering
874249f
@jasontedor
Imports
2d4ab87
@jasontedor
Merge remote-tracking branch 'elastic/master' into java-11-collection…
62d7b7f 
…s-in-all-the-places

* elastic/master: (70 commits)
  Remove experimental label froms script_score query (elastic#41572)
  [Ml-Dataframe] Update URLs in Data frame client java doc (elastic#41539)
  Reenable bwc Tests in master (elastic#41540)
  Fix search_as_you_type's sub-fields to pick their names from the full path of the root field (elastic#41541)
  Remove search analyzers from DocumentFieldMappers (elastic#41484)
  Update community client and integration docs (elastic#41513)
  Remove Version.V_6_x_x constants use in security (elastic#41185)
  Mute testDriverConfigurationWithSSLInURL
  Remove dedicated SSL network write buffer (elastic#41283)
  Disable max score optimization for queries with unbounded max scores (elastic#41361)
  [DOCS] Explicitly set section IDs for Asciidoctor migration (elastic#41547)
  [ML] add multi node integ tests for data frames (elastic#41508)
  [Docs] Fix common word repetitions (elastic#39703)
  [DOCS] Note TESTRESPONSE can't be used immediately after TESTSETUP (elastic#41542)
  Update configuring-ldap-realm.asciidoc (elastic#40427)
  Fixed very small typo in date (elastic#41398)
  Refactor GeoHashUtils (elastic#40869)
  Make 0 as invalid value for `min_children` in `has_child` query (elastic#41347)
  field_caps: adapt bwc version after backport (elastic#41427)
  Remove Exists Check from S3 Repository Deletes (elastic#40931)
  ...
@jasontedor
Copy link
Member Author

@elasticmachine run elasticsearch-ci/1

@jasontedor jasontedor merged commit f48ddd5 into elastic:master Apr 26, 2019
@jasontedor jasontedor deleted the java-11-collections-in-all-the-places branch April 26, 2019 16:32
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
@jasontedor @akhil10x5
Use Java 11 collections conveniences everywhere (elastic#41399)
1357fa9 
This commit replaces all applicable uses with Java 11 collections
convenience methods.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@jasontedor
Use Java 11 collections conveniences everywhere (elastic#41399)
0fb4ada 
This commit replaces all applicable uses with Java 11 collections
convenience methods.
@tvernum tvernum mentioned this pull request Oct 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasontedor @elasticmachine @rjernst @jpountz @jakelandis