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

Introduce ShardState Enum + Slight Cleanup SnapshotsInProgress #41940

Merged

Conversation

original-brownbear
Copy link
Member

  • Added separate enum for the state of each shard, it was really
    confusing that we used the same enum for the state of the snapshot
    overall and the state of each individual shard
  • Moved the contents of the class around a little so fields,
    constructors and nested classes/enums aren't all over the place
    especially now that we have yet another nested enum here
  • Shortened some obvious spots in equals method and saved a few lines
    via computeIfAbsent to make up for adding 50 new lines to this class

* Added separate enum for the state of each shard, it was really
confusing that we used the same enum for the state of the snapshot
overall and the state of each individual shard
   * relates elastic#40943 (comment)
* Moved the contents of the class around a little so fields,
constructors and nested classes/enums aren't all over the place
especially now that we have yet another nested enum here
* Shortened some obvious spots in equals method and saved a few lines
via `computeIfAbsent` to make up for adding 50 new lines to this class
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/1

Copy link
Contributor

@andrershov andrershov left a comment

Choose a reason for hiding this comment

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

I'd rather rename this PR to introduce ShardState enum, because it's not so vague as Cleanup SnapshotsInProgress. I think it's ok if computeIfAbsent change will be part of this PR.

@@ -42,23 +42,78 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;

/**
* Meta data about snapshots that are currently executing
*/
public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implements Custom {
Copy link
Contributor

Choose a reason for hiding this comment

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

This diff is not readable, can you please move things back in the file. It would be much easier for the reviewers to see what exactly was changed and you won't be displayed in git history as the author for the lines that you have just re-shuffled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure we can shuffle that later :), reverted that part and fixed PR title.

@original-brownbear original-brownbear changed the title Cleanup SnapshotsInProgress Introduce ShardState Enum + Slight Cleanup SnapshotsInProgress May 9, 2019
@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/2
Jenkins run elasticsearch-ci/packaging-sample

@original-brownbear
Copy link
Member Author

original-brownbear commented May 20, 2019

@andrershov ping :) (bwc is apparently broken due to the version bumps right now)

Copy link
Contributor

@andrershov andrershov left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/1
Jenkins run elasticsearch-ci/bwc

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc

1 similar comment
@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc

@original-brownbear original-brownbear merged commit 2ddd39a into elastic:master May 22, 2019
@original-brownbear original-brownbear deleted the cleanup-shard-state-enum branch May 22, 2019 07:47
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request May 27, 2019
…ic#41940)

* Added separate enum for the state of each shard, it was really
confusing that we used the same enum for the state of the snapshot
overall and the state of each individual shard
   * relates elastic#40943 (comment)
* Shortened some obvious spots in equals method and saved a few lines
via `computeIfAbsent` to make up for adding 50 new lines to this class
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
…ic#41940)

* Added separate enum for the state of each shard, it was really
confusing that we used the same enum for the state of the snapshot
overall and the state of each individual shard
   * relates elastic#40943 (comment)
* Shortened some obvious spots in equals method and saved a few lines
via `computeIfAbsent` to make up for adding 50 new lines to this class
original-brownbear added a commit that referenced this pull request May 27, 2019
… (#42573)

* Added separate enum for the state of each shard, it was really
confusing that we used the same enum for the state of the snapshot
overall and the state of each individual shard
   * relates #40943 (comment)
* Shortened some obvious spots in equals method and saved a few lines
via `computeIfAbsent` to make up for adding 50 new lines to this class
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Apr 7, 2020
This is a left-over from before elastic#41940 when we used the same status enum for the shards
and the snapshots overall. The two removed values were never used on the shard level
so we can simply remove them here.
original-brownbear added a commit that referenced this pull request Apr 7, 2020
* Remove Unused Snapshot Status Values

This is a left-over from before #41940 when we used the same status enum for the shards
and the snapshots overall. The two removed values were never used on the shard level
so we can simply remove them here.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Apr 7, 2020
* Remove Unused Snapshot Status Values

This is a left-over from before elastic#41940 when we used the same status enum for the shards
and the snapshots overall. The two removed values were never used on the shard level
so we can simply remove them here.
original-brownbear added a commit that referenced this pull request Apr 7, 2020
* Remove Unused Snapshot Status Values

This is a left-over from before #41940 when we used the same status enum for the shards
and the snapshots overall. The two removed values were never used on the shard level
so we can simply remove them here.
mkleen added a commit to crate/crate that referenced this pull request May 26, 2020
mkleen added a commit to crate/crate that referenced this pull request May 26, 2020
mkleen added a commit to crate/crate that referenced this pull request May 27, 2020
mkleen added a commit to crate/crate that referenced this pull request May 27, 2020
mkleen added a commit to crate/crate that referenced this pull request May 27, 2020
mkleen added a commit to crate/crate that referenced this pull request May 27, 2020
mergify bot pushed a commit to crate/crate that referenced this pull request May 27, 2020
@original-brownbear original-brownbear restored the cleanup-shard-state-enum branch August 6, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants