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

Create specific exception for when snapshots are in progress #37550

Merged
merged 4 commits into from
Jan 17, 2019
Merged

Create specific exception for when snapshots are in progress #37550

merged 4 commits into from
Jan 17, 2019

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Jan 16, 2019

delete and close index actions threw IllegalArgumentExceptions when
attempting to run against an index that has a snapshot in progress.

This change introduces a dedicated SnapshotInProgressException for
these scenarios. This is done to explicitly signal to clients that
this is the reason the action failed, and it is a retryable error.

relates to #37541.

@talevy talevy added the :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Jan 16, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@talevy talevy force-pushed the snapshot-in-progress-exception branch from 6ffff88 to f70c0b6 Compare January 16, 2019 18:26
@talevy
Create specific exception for when snapshots are in progress
f713336 
delete and close index actions threw IllegalArgumentExceptions when
attempting to run against an index that has a snapshot in progress.

This change introduces a dedicated SnapshotInProgressException for
these scenarios. This is done to explicitely signal to clients that
this is the reason the action failed, and it is a retryable error.

relates to elastic#37541.
@talevy talevy force-pushed the snapshot-in-progress-exception branch from f70c0b6 to f713336 Compare January 16, 2019 18:27
@talevy talevy mentioned this pull request Jan 16, 2019
Closed
@talevy
Copy link
Contributor Author

talevy commented Jan 16, 2019

run gradle build tests 2

dakrone
dakrone approved these changes Jan 16, 2019
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

This LGTM but should have @original-brownbear take a look before merging.

@talevy
Copy link
Contributor Author

talevy commented Jan 17, 2019

run gradle build tests 1

original-brownbear
original-brownbear approved these changes Jan 17, 2019
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM (assuming CI goes green eventually), one question/NIT :)

server/src/main/java/org/elasticsearch/snapshots/SnapshotInProgressException.java Outdated Show resolved Hide resolved
@talevy
Copy link
Contributor Author

talevy commented Jan 17, 2019

thanks for the reviews @dakrone and @original-brownbear!

@talevy
Copy link
Contributor Author

talevy commented Jan 17, 2019

run gradle build tests 2

@talevy talevy merged commit a0c504e into elastic:master Jan 17, 2019
@talevy talevy deleted the snapshot-in-progress-exception branch January 17, 2019 21:21
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 17, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into thread-context-add…
c801d39 
…-response-header-performance

* elastic/master:
  Remove Redundant RestoreRequest Class (elastic#37535)
  Create specific exception for when snapshots are in progress (elastic#37550)
  Mute UnicastZenPingTests#testSimplePings
  [DOCS] Adds size limitation to the get datafeeds APIs (elastic#37578)
  Fix assertion at end of forceRefreshes (elastic#37559)
  Propagate Errors in executors to uncaught exception handler (elastic#36137)
  [DOCS] Adds limitation to the get jobs API (elastic#37549)
  Add set_priority action to ILM (elastic#37397)
  Make recovery source send operations non-blocking (elastic#37503)
  Allow field types to optimize phrase prefix queries (elastic#37436)
  Added fatal_exception field for ccr stats in monitoring mapping. (elastic#37563)
  Fix testRelocateWhileContinuouslyIndexingAndWaitingForRefresh (elastic#37560)
  Moved ccr integration to the package with other ccr integration tests.
  Mute TransportClientNodesServiceTests#testListenerFailures
  Decreased time out in test
  Fix erroneous docstrings for abstract bulk by scroll request (elastic#37517)
  SQL: Rename SQL type DATE to DATETIME (elastic#37395)
  Remove the AbstracLifecycleComponent constructor with Settings (elastic#37523)
@DaveCTurner
Copy link
Contributor

@talevy we intend to delete ClusterAlreadyBootstrappedException in #37463, and in merging master into that one I noticed a conflict with this PR which adds SnapshotInProgressException. I think since neither exists in a released version we should renumber SnapshotInProgressException as if ClusterAlreadyBootstrappedException never existed, and that's what I've done in #37463 to resolve the merge conflict, but I note that this PR will be backported so maybe it's going to keep things simpler if we keep the numbering as-is. WDYT?

@talevy
Copy link
Contributor Author

talevy commented Jan 18, 2019

@DaveCTurner I see that change. the plan sounds reasonable to me. I will wait for this change to go in before backporting

talevy added a commit to talevy/elasticsearch that referenced this pull request Jan 22, 2019
@talevy
Create specific exception for when snapshots are in progress (elastic…
0c2737f 
…#37550)

delete and close index actions threw IllegalArgumentExceptions
when attempting to run against an index that has a snapshot
in progress.

This change introduces a dedicated SnapshotInProgressException
for these scenarios. This is done to explicitly signal to clients that
this is the reason the action failed, and it is a retryable error.

relates to elastic#37541.
@talevy talevy mentioned this pull request Jan 22, 2019
Merged
talevy added a commit that referenced this pull request Jan 23, 2019
@talevy
Create specific exception for when snapshots are in progress (#37550) (…
d3cfa61 
…#37723)

delete and close index actions threw IllegalArgumentExceptions
when attempting to run against an index that has a snapshot
in progress.

This change introduces a dedicated SnapshotInProgressException
for these scenarios. This is done to explicitly signal to clients that
this is the reason the action failed, and it is a retryable error.

relates to #37541.
@DaveCTurner
Copy link
Contributor

@talevy looks like this was backported now, but does the version need adjusting in the deserialiser?

elasticsearch/server/src/main/java/org/elasticsearch/ElasticsearchException.java

Line 1013 in 170d741

org.elasticsearch.snapshots.SnapshotInProgressException::new, 151, Version.V_7_0_0),

I'm also wondering whether we're missing a test somewhere, as I would have expected the mixed-cluster tests to pick this up.

@talevy
Copy link
Contributor Author

talevy commented Jan 25, 2019

@DaveCTurner thanks for reminding me, I made a note to follow-up on this, but forgot. I noticed
that this is not the only Exception that has these version discrepancy. TooManyBucketsException also is set to 7.0. I will open a PR. No clue about tests. we can discuss on the follow-up PR

@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@original-brownbear original-brownbear original-brownbear approved these changes

@dakrone dakrone dakrone approved these changes

Assignees
No one assigned
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@talevy @elasticmachine @DaveCTurner @dakrone @original-brownbear @colings86 @danielmitterdorfer