Skip to content

Create specific exception for when snapshots are in progress#37550

Merged
talevy merged 4 commits intoelastic:masterfrom
talevy:snapshot-in-progress-exception
Jan 17, 2019
Merged

Create specific exception for when snapshots are in progress#37550
talevy merged 4 commits intoelastic:masterfrom
talevy:snapshot-in-progress-exception

Conversation

@talevy
Copy link
Copy Markdown
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/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Jan 16, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jan 16, 2019

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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 :)

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 17, 2019

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

@talevy
Copy link
Copy Markdown
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
Copy Markdown
Member

DaveCTurner commented Jan 18, 2019

@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
Copy Markdown
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
Copy Markdown
Member

DaveCTurner commented Jan 25, 2019

@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
Copy Markdown
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

@dakrone dakrone dakrone approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Distributed/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