Skip to content

Allow empty/missing SLM retention configuration #45018

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

Merged
merged 2 commits into from
Jul 31, 2019
Merged

Allow empty/missing SLM retention configuration #45018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ba2fcc
Allow empty/missing SLM retention configuration
dakrone Jul 30, 2019
1fa5914
Merge branch 'slm-retention' into slm-allow-missing-retention
elasticmachine Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions ...t/rest-high-level/src/main/java/org/elasticsearch/client/slm/SnapshotLifecyclePolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ public class SnapshotLifecyclePolicy implements ToXContentObject {
PARSER.declareString(ConstructingObjectParser.constructorArg(), SCHEDULE);
PARSER.declareString(ConstructingObjectParser.constructorArg(), REPOSITORY);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> p.map(), CONFIG);
PARSER.declareObject(ConstructingObjectParser.constructorArg(), SnapshotRetentionConfiguration::parse, RETENTION);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), SnapshotRetentionConfiguration::parse, RETENTION);
}

public SnapshotLifecyclePolicy(final String id, final String name, final String schedule,
final String repository, @Nullable Map<String, Object> configuration,
SnapshotRetentionConfiguration retentionPolicy) {
this.id = Objects.requireNonNull(id);
this.name = name;
this.schedule = schedule;
this.repository = repository;
final String repository, @Nullable final Map<String, Object> configuration,
@Nullable final SnapshotRetentionConfiguration retentionPolicy) {
this.id = Objects.requireNonNull(id, "policy id is required");
this.name = Objects.requireNonNull(name, "policy snapshot name is required");
this.schedule = Objects.requireNonNull(schedule, "policy schedule is required");
this.repository = Objects.requireNonNull(repository, "policy snapshot repository is required");
this.configuration = configuration;
this.retentionPolicy = retentionPolicy;
}
Expand All @@ -98,6 +98,7 @@ public Map<String, Object> getConfig() {
return this.configuration;
}

@Nullable
public SnapshotRetentionConfiguration getRetentionPolicy() {
return this.retentionPolicy;
}
Expand All @@ -115,7 +116,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (this.configuration != null) {
builder.field(CONFIG.getPreferredName(), this.configuration);
}
builder.field(RETENTION.getPreferredName(), this.retentionPolicy);
if (this.retentionPolicy != null) {
builder.field(RETENTION.getPreferredName(), this.retentionPolicy);
}
builder.endObject();
return builder;
}
Expand Down
15 changes: 9 additions & 6 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotLifecyclePolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ public class SnapshotLifecyclePolicy extends AbstractDiffable<SnapshotLifecycleP
PARSER.declareString(ConstructingObjectParser.constructorArg(), SCHEDULE);
PARSER.declareString(ConstructingObjectParser.constructorArg(), REPOSITORY);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> p.map(), CONFIG);
PARSER.declareObject(ConstructingObjectParser.constructorArg(), SnapshotRetentionConfiguration::parse, RETENTION);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), SnapshotRetentionConfiguration::parse, RETENTION);
}

public SnapshotLifecyclePolicy(final String id, final String name, final String schedule,
final String repository, @Nullable Map<String, Object> configuration,
final SnapshotRetentionConfiguration retentionPolicy) {
final String repository, @Nullable final Map<String, Object> configuration,
@Nullable final SnapshotRetentionConfiguration retentionPolicy) {
this.id = Objects.requireNonNull(id, "policy id is required");
this.name = Objects.requireNonNull(name, "policy snapshot name is required");
this.schedule = Objects.requireNonNull(schedule, "policy schedule is required");
Expand All @@ -102,7 +102,7 @@ public SnapshotLifecyclePolicy(StreamInput in) throws IOException {
this.schedule = in.readString();
this.repository = in.readString();
this.configuration = in.readMap();
this.retentionPolicy = new SnapshotRetentionConfiguration(in);
this.retentionPolicy = in.readOptionalWriteable(SnapshotRetentionConfiguration::new);
Copy link
Contributor

@gwbrown gwbrown Jul 30, 2019

Choose a reason for hiding this comment

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

I didn't notice this the first time around but we'll probably need to version-gate this if there's a version of SLM that ships that doesn't have retention.

Copy link
Member Author

@dakrone dakrone Jul 30, 2019

Choose a reason for hiding this comment

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

+1 on addressing this, but probably not until we're about to merge

}

public String getId() {
Expand All @@ -126,6 +126,7 @@ public Map<String, Object> getConfig() {
return this.configuration;
}

@Nullable
public SnapshotRetentionConfiguration getRetentionPolicy() {
return this.retentionPolicy;
}
Expand Down Expand Up @@ -271,7 +272,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(this.schedule);
out.writeString(this.repository);
out.writeMap(this.configuration);
this.retentionPolicy.writeTo(out);
out.writeOptionalWriteable(this.retentionPolicy);
}

@Override
Expand All @@ -283,7 +284,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (this.configuration != null) {
builder.field(CONFIG.getPreferredName(), this.configuration);
}
builder.field(RETENTION.getPreferredName(), this.retentionPolicy);
if (this.retentionPolicy != null) {
builder.field(RETENTION.getPreferredName(), this.retentionPolicy);
}
builder.endObject();
return builder;
}
Expand Down
2 changes: 1 addition & 1 deletion .../src/test/java/org/elasticsearch/xpack/core/slm/SnapshotLifecyclePolicyMetadataTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static SnapshotLifecyclePolicy randomSnapshotLifecyclePolicy(String polic
}

public static SnapshotRetentionConfiguration randomRetention() {
return new SnapshotRetentionConfiguration(rarely() ? null :
return rarely() ? null : new SnapshotRetentionConfiguration(rarely() ? null :
TimeValue.parseTimeValue(randomTimeValue(), "random retention generation"));
}

Expand Down