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

Simplify Snapshot Create Request Handling #37464

Merged

Conversation

original-brownbear
Copy link
Member

  • The internal create request is absolutely redundant, the only difference to the transport request is that we resolved the snapshot
    name when moving from the transport to the internal version
    • Removed it and passed the transport request into the snapshot service instead

* The internal create request is absolutely redundant, the only difference to the transport request is that we resolved the snapshot
name when moving from the transport to the internal version
  * Removed it and passed the transport request into the snapshot service instead
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

final String snapshotName = request.snapshotName;
public void createSnapshot(final CreateSnapshotRequest request, final CreateSnapshotListener listener) {
final String repositoryName = request.repository();
final String snapshotName = indexNameExpressionResolver.resolveDateMathExpression(request.snapshot());
Copy link
Contributor

Choose a reason for hiding this comment

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

we resolve this twice now, and if you have stuff like now in it, it might resolve to two different values. Perhaps we can wrap the original request?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this instead 27ca06b ? Just pass the snapshot back in the callback and get a cleaner equals in the transport action as well? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

++

@original-brownbear
Copy link
Member Author

Jenkins retest this please

@original-brownbear
Copy link
Member Author

Jenkins run gradle build tests 1

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

@ywelsch thanks!

@original-brownbear original-brownbear merged commit 5a5e44d into elastic:master Jan 16, 2019
@original-brownbear original-brownbear deleted the simplify-snapshot-service branch January 16, 2019 10:08
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jan 16, 2019
* Same as elastic#37464 but for the restore side
cshtdd added a commit to cshtdd/elasticsearch that referenced this pull request Jan 16, 2019
* master:
  Deprecate _type from LeafDocLookup (elastic#37491)
  Allow system privilege to execute proxied actions (elastic#37508)
  Update Put Watch to allow unknown fields (elastic#37494)
  AwaitsFix testAddNewReplicas
  SQL: Add protocol tests and remove jdbc_type from drivers response (elastic#37516)
  SQL: [Docs] Add an ES-SQL column for data types (elastic#37529)
  IndexMetaData#mappingOrDefault doesn't need to take a type argument. (elastic#37480)
  Simplify + Cleanup Dead Code in Settings (elastic#37341)
  Reject all requests that have an unconsumed body (elastic#37504)
  [Ml] Prevent config snapshot failure blocking migration (elastic#37493)
  Fix line length for aliases and remove suppression (elastic#37455)
  Add SSL Configuration Library (elastic#37287)
  SQL: Remove slightly used meta commands (elastic#37506)
  Simplify Snapshot Create Request Handling (elastic#37464)
  Remove the use of AbstracLifecycleComponent constructor elastic#37488 (elastic#37488)
  [ML] log minimum diskspace setting if forecast fails due to insufficient d… (elastic#37486)
original-brownbear added a commit that referenced this pull request Jan 17, 2019
* Same as #37464 but for the restore side
original-brownbear added a commit that referenced this pull request Jan 28, 2019
* The internal create request is absolutely redundant, the only difference to the transport request is that we resolved the snapshot
name when moving from the transport to the internal version
  * Removed it and passed the transport request into the snapshot service instead
* nicer way of resolve snapshot name in callback
original-brownbear added a commit that referenced this pull request Jan 28, 2019
* Same as #37464 but for the restore side
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.

4 participants