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

Remove ImmutableOpenMap from snapshot services #90006

Merged
merged 4 commits into from
Sep 22, 2022
Merged

Remove ImmutableOpenMap from snapshot services #90006

merged 4 commits into from
Sep 22, 2022

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Sep 12, 2022 •
edited
Loading

Part of #86239

@thecoop thecoop force-pushed the immutablemap-snapshots branch from bddfa8c to 0e272a6 Compare September 12, 2022 13:47
thecoop
thecoop commented Sep 12, 2022
View reviewed changes
server/src/main/java/org/elasticsearch/snapshots/InternalSnapshotsInfoService.java
@@ -64,7 +65,7 @@ public class InternalSnapshotsInfoService implements ClusterStateListener, Snaps
private final Supplier<RerouteService> rerouteService;

/** contains the snapshot shards for which the size is known **/
private volatile ImmutableOpenMap<SnapshotShard, Long> knownSnapshotShards;
private volatile Map<SnapshotShard, Long> knownSnapshotShards;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is performance an important factor in this class?

@thecoop thecoop marked this pull request as ready for review September 12, 2022 14:38
@thecoop thecoop added :Core/Infra/Core Core issues without another label >refactoring labels Sep 12, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 12, 2022
@elasticsearchmachine
Copy link
Collaborator

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

grcevski
grcevski reviewed Sep 12, 2022
View reviewed changes
server/src/main/java/org/elasticsearch/snapshots/InternalSnapshotsInfoService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/RestoreService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Outdated Show resolved Hide resolved
grcevski
grcevski reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@grcevski grcevski 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 iterating on this Simon. Can we please separate the improvements to InternalSnapshotInfoService into a separate PR and keep this PR focused on ImmutableOpenMap?

server/src/main/java/org/elasticsearch/cluster/RestoreInProgress.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/InternalSnapshotsInfoService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/InternalSnapshotsInfoService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
@thecoop thecoop mentioned this pull request Sep 15, 2022
Merged
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
grcevski
grcevski approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! I left few comments that I think we should address.

server/src/main/java/org/elasticsearch/snapshots/InternalSnapshotsInfoService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/RestoreService.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Show resolved Hide resolved
@thecoop thecoop merged commit a5f41a8 into elastic:main Sep 22, 2022
@thecoop thecoop deleted the immutablemap-snapshots branch September 23, 2022 08:35
@thecoop thecoop mentioned this pull request Sep 23, 2022
Open
35 tasks
thecoop added a commit that referenced this pull request Sep 23, 2022
@thecoop
Revert "Remove ImmutableOpenMap from snapshot services (#90006)"
c91e89f 
This reverts commit a5f41a8.
@thecoop thecoop mentioned this pull request Sep 23, 2022
Merged
thecoop added a commit that referenced this pull request Sep 23, 2022
@thecoop
Revert "Remove ImmutableOpenMap from snapshot services" (#90287)
e1c284a 
* Revert "Remove ImmutableOpenMap from snapshot services (#90006)"

This reverts commit a5f41a8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grcevski grcevski grcevski approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thecoop @elasticsearchmachine @grcevski @csoulios