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

Allow date math for naming newly-created snapshots (#7939) #30479

Merged
merged 8 commits into from May 16, 2018

Conversation

Projects
None yet
5 participants
@vladimirdolzhenko
Copy link
Contributor

commented May 9, 2018

Use date math expression (same as used for indices naming) for naming newly-created snapshots

Vladimir Dolzhenko
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2018

@ywelsch
Copy link
Contributor

left a comment

Can you add a description to your PR to explain what it does? Also remove the backport_pending label please, it should only be used if you've merged the PR into master but not backported the change (e.g. due to some complication)

@@ -282,7 +282,8 @@ PUT /_snapshot/my_backup/snapshot_2?wait_for_completion=true
// TEST[continued]

The list of indices that should be included into the snapshot can be specified using the `indices` parameter that
supports <<multi-index,multi index syntax>>. The snapshot request also supports the
supports <<multi-index,multi index syntax>>. It can be useful to use <<date-math-index-names,date math>> to name the

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 9, 2018

Contributor

This links to docs on using date math for index names. This can be confusing. I think you need to clarify here that the same date math expression that is used for indices can be used for snapshot names. I think it would be better to put this info into a separate paragraph, together with an example. I wonder if we need to support date math not only for snapshot creation, but also for other operations.

@@ -176,4 +177,30 @@ public void testSnapshotStatusWithBlocks() {
setClusterReadOnly(false);
}
}

public void testSnapshotWithDateMath() {

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 9, 2018

Contributor

I'm not sure what this is exactly testing. There are no assertions that check that the date math functionality has worked in a meaningful way. Also why is this testing blocks?

This comment has been minimized.

Copy link
@vladimirdolzhenko

vladimirdolzhenko May 11, 2018

Author Contributor

it tests that it is possible to create snapshot with name <snapshot-{now/d}> and it is resolved into smth like snapshot-2018.05.11 using nameExpressionResolver.resolveDateMathExpression(snapshotName) - I assume that nameExpressionResolver works.

Agree - it is worth to move test to DedicatedClusterSnapshotRestoreIT - is that ok ?

Vladimir Dolzhenko added some commits May 11, 2018

Vladimir Dolzhenko
Automatic snapshot naming #7939
move test case to DedicatedClusterSnapshotRestoreIT
Vladimir Dolzhenko
Automatic snapshot naming #7939
extended doc: example is added. referring to indices date math more clearly
@ywelsch
Copy link
Contributor

left a comment

I've left a few more comments. Can you also adapt the PR title so that it reflects more closely what's implemented here:

Allow date math for naming newly-created snapshots

and make it clear in the description that this only allows date math when creating new snapshots (it does not implement date math when resolving snapshot names, for exampling when asking for the snapshots that currently exist in the repo).

@@ -176,4 +176,6 @@ public void testSnapshotStatusWithBlocks() {
setClusterReadOnly(false);
}
}


This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

can you revert this?


final IndexNameExpressionResolver nameExpressionResolver = new IndexNameExpressionResolver(Settings.EMPTY);
final String snapshotName = "<snapshot-{now/d}>";
final String expression = nameExpressionResolver.resolveDateMathExpression(snapshotName);

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

I think that if there is a day rollover between resolving the expression here and taking the snapshot, the test will fail. A fix might be to resolve the expression a second time after the snapshot was taken, and then use both resolved expressions when asking for the snapshot status.

admin.cluster().prepareCreateSnapshot(repo, snapshotName)
.setIncludeGlobalState(true)
.setWaitForCompletion(true)
.execute().actionGet();

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

just .get() will do. Also no need for setIncludeGlobalState(true), that's irrelevant for the test.

This comment has been minimized.

Copy link
@vladimirdolzhenko

vladimirdolzhenko May 15, 2018

Author Contributor

agreed, just copy-n-pasted from another place

@@ -289,6 +289,19 @@ By setting `include_global_state` to false it's possible to prevent the cluster
the snapshot. By default, the entire snapshot will fail if one or more indices participating in the snapshot don't have
all primary shards available. This behaviour can be changed by setting `partial` to `true`.

Snapshot could be named according to the date that the snapshot made in a similar way as indices

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

How about the following:

Snapshot names can be automatically derived using <<date-math-index-names,date math expressions>>, similarly as when creating new indices. Note that special characters need to be URI encoded. For example, creating a snapshot with the current day in the name, like snapshot-2018.05.11, can be achieved with the following command:

...

@vladimirdolzhenko vladimirdolzhenko changed the title Automatic snapshot naming (#7939) Allow date math for naming newly-created snapshots (#7939) May 15, 2018

Vladimir Dolzhenko added some commits May 15, 2018

@vladimirdolzhenko vladimirdolzhenko requested a review from ywelsch May 15, 2018

@vladimirdolzhenko

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

@ywelsch thank you for the feedback - I made relevant amendments.

@ywelsch
Copy link
Contributor

left a comment

Thanks @vladimirdolzhenko - LGTM. I've added version labels to the PR.

final String expression2 = nameExpressionResolver.resolveDateMathExpression(snapshotName);

SnapshotsStatusResponse response = admin.cluster().prepareSnapshotStatus(repo)
.setSnapshots(Sets.newHashSet(expression1, expression2).toArray(Strings.EMPTY_ARRAY))

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

this is a varargs method, so just .setSnapshots(expression1, expression2) is good enough.

This comment has been minimized.

Copy link
@vladimirdolzhenko

vladimirdolzhenko May 15, 2018

Author Contributor

w/o a day rollover it requests two statuses with two identical names - and the result is 2 snapshot statuses - that's why I used set here

This comment has been minimized.

Copy link
@ywelsch

ywelsch May 15, 2018

Contributor

ok

@jasontedor

This comment has been minimized.

Copy link
Member

commented May 15, 2018

test this please

@vladimirdolzhenko vladimirdolzhenko merged commit fe3e025 into elastic:master May 16, 2018

3 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@vladimirdolzhenko vladimirdolzhenko deleted the vladimirdolzhenko:7939 branch May 16, 2018

vladimirdolzhenko pushed a commit that referenced this pull request May 16, 2018

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 16, 2018

Merge remote-tracking branch 'elastic/6.x' into hard-coded-ports-are-…
…bad-mkay

* elastic/6.x:
  [ML] Wait for ML indices in rolling upgrade tests (elastic#30615)
  Watcher: Ensure secrets integration tests also run triggered watch (elastic#30478)
  Move allocation awareness attributes to list setting (elastic#30626)
  [Docs] Update code snippet in has-child-query.asciidoc (elastic#30510)
  Allow date math for naming newly-created snapshots (elastic#7939) (elastic#30479)
  Awaits fix a failing test
  Switch many QA projects to use new style requests (elastic#30574)
  QA: System property to override distribution (elastic#30591)
@vladimirdolzhenko

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2018

thanks @ywelsch

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.