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

[Refactor] SegmentsStats#filesSizes from ImmutableOpenMap to java.util.Map #7094

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Apr 11, 2023

Using an ImmutableOpenMap in o.o.index.engine.SegmentsStats#filesSizes is overkill since all we care about is public facing API not leaking the filesSizes collection. To remove all of the unnecessary temporary object copies saddled with using ImmutableOpenMap internally, this PR refactors the SegmentsStats#filesSizes variable from an ImmutableOpenMap to an unmodifiable java.util.Map. It also refactors the internal only used FILE_DESCRIPTIONS static variable to an unmodifiable java.util.Map in order to completely remove SegmentsStats dependency on hppc based ImmutableOpenMap.

relates #5910

…l.Map

Using an ImmutableOpenMap in filesSizes in o.o.index.engine.SegmentsStats
is overkill since all we care about is public facing API not leaking the
filesSizes collection. This commit refactors the SegmentsStats#filesSizes
variable from an ImmutableOpenMap to an unmodifiable java.util.Map to
remove all of the unnecessary temporary object copies on the internal
map.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member

Gradle Check (Jenkins) Run Completed with:

Doc count assertion failure, seems non-related to changes. Retrying check.

REPRODUCE WITH: ./gradlew ':modules:geo:internalClusterTest' --tests "org.opensearch.geo.search.aggregations.bucket.GeoTileGridIT.testMultivaluedGeoPointsAggregation" -Dtests.seed=73A6EB980B2F13B4 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=is -Dtests.timezone=Africa/Addis_Ababa -Druntime.java=19

org.opensearch.geo.search.aggregations.bucket.GeoTileGridIT > testMultivaluedGeoPointsAggregation FAILED
    java.lang.AssertionError: Geohash 13/1627/7465 has wrong doc count  expected:<0> but was:<1>
        at __randomizedtesting.SeedInfo.seed([73A6EB980B2F13B4:BE14626187F3995E]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.opensearch.geo.search.aggregations.bucket.GeoTileGridIT.testMultivaluedGeoPointsAggregation(GeoTileGridIT.java:128)

Copy link
Collaborator

@reta reta left a comment

Choose a reason for hiding this comment

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

Nice (Spring?) cleanup :)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.index.WaitUntilRefreshIT.classMethod

@codecov-commenter
Copy link

Codecov Report

Merging #7094 (fce11ab) into main (64f3123) will decrease coverage by 0.30%.
The diff coverage is 75.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #7094      +/-   ##
============================================
- Coverage     71.10%   70.80%   -0.30%     
+ Complexity    59808    59485     -323     
============================================
  Files          4852     4852              
  Lines        285138   285123      -15     
  Branches      41107    41104       -3     
============================================
- Hits         202755   201892     -863     
- Misses        66040    66628     +588     
- Partials      16343    16603     +260     
Impacted Files Coverage Δ
.../main/java/org/opensearch/index/engine/Engine.java 74.09% <40.00%> (-2.06%) ⬇️
...ava/org/opensearch/index/engine/SegmentsStats.java 72.41% <80.00%> (+1.42%) ⬆️

... and 446 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nknize
Copy link
Collaborator Author

nknize commented Apr 11, 2023

I opened #7101 for the unrelated GeoHashGridIT failure. Rerunning the cancelled precommit job and will merge on green.

@nknize nknize merged commit 352de4e into opensearch-project:main Apr 11, 2023
@andrross andrross added the backport 2.x Backport to 2.x branch label Apr 14, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 14, 2023
…l.Map (#7094)

Using an ImmutableOpenMap in filesSizes in o.o.index.engine.SegmentsStats
is overkill since all we care about is public facing API not leaking the
filesSizes collection. This commit refactors the SegmentsStats#filesSizes
variable from an ImmutableOpenMap to an unmodifiable java.util.Map to
remove all of the unnecessary temporary object copies on the internal
map.

It also refactors the internal only used FILE_DESCRIPTIONS static variable
to an unmodifiable java.util.Map in order to completely remove SegmentsStats
dependency on hppc based ImmutableOpenMap.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
(cherry picked from commit 352de4e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross pushed a commit that referenced this pull request Apr 14, 2023
…l.Map (#7094) (#7162)

Using an ImmutableOpenMap in filesSizes in o.o.index.engine.SegmentsStats
is overkill since all we care about is public facing API not leaking the
filesSizes collection. This commit refactors the SegmentsStats#filesSizes
variable from an ImmutableOpenMap to an unmodifiable java.util.Map to
remove all of the unnecessary temporary object copies on the internal
map.

It also refactors the internal only used FILE_DESCRIPTIONS static variable
to an unmodifiable java.util.Map in order to completely remove SegmentsStats
dependency on hppc based ImmutableOpenMap.


(cherry picked from commit 352de4e)

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Apr 28, 2023
…l.Map (opensearch-project#7094)

Using an ImmutableOpenMap in filesSizes in o.o.index.engine.SegmentsStats
is overkill since all we care about is public facing API not leaking the
filesSizes collection. This commit refactors the SegmentsStats#filesSizes
variable from an ImmutableOpenMap to an unmodifiable java.util.Map to
remove all of the unnecessary temporary object copies on the internal
map. 

It also refactors the internal only used FILE_DESCRIPTIONS static variable 
to an unmodifiable java.util.Map in order to completely remove SegmentsStats 
dependency on hppc based ImmutableOpenMap.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch enhancement Enhancement or improvement to existing feature or request skip-changelog v2.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants