Skip to content

copy LifecyclePolicy to protocol.xpack #32915

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 5 commits into from
Aug 20, 2018
Merged

copy LifecyclePolicy to protocol.xpack #32915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6bb033
copy LifecyclePolicy to protocol.xpack
talevy Aug 16, 2018
a2f675f
respond to review comments
talevy Aug 16, 2018
7d76855
Merge branch 'index-lifecycle' into ilm-protocol-policy
talevy Aug 16, 2018
880e3f9
Merge branch 'index-lifecycle' into ilm-protocol-policy
talevy Aug 17, 2018
8c764b4
move validate inline
talevy Aug 17, 2018
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
15 changes: 1 addition & 14 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,20 +440,7 @@ public List<NamedXContentRegistry.Entry> getNamedXContent() {
RollupJobStatus::fromXContent),
new NamedXContentRegistry.Entry(PersistentTaskState.class, new ParseField(RollupJobStatus.NAME),
RollupJobStatus::fromXContent),
// ILM - Custom Metadata
new NamedXContentRegistry.Entry(MetaData.Custom.class, new ParseField(IndexLifecycleMetadata.TYPE),
parser -> IndexLifecycleMetadata.PARSER.parse(parser, null)),
// ILM - Lifecycle Types
new NamedXContentRegistry.Entry(LifecycleType.class, new ParseField(TimeseriesLifecycleType.TYPE),
(p, c) -> TimeseriesLifecycleType.INSTANCE),
// ILM - Lifecycle Actions
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(AllocateAction.NAME), AllocateAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ForceMergeAction.NAME), ForceMergeAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ReadOnlyAction.NAME), ReadOnlyAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(RolloverAction.NAME), RolloverAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ShrinkAction.NAME), ShrinkAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(DeleteAction.NAME), DeleteAction::parse),
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that I previously said these were no longer needed to be registered here but I'm actually wondering now if the actions and LifecycleType need to be registered here for the transport client? I think because the Action class will still reference these and the transport client uses the action objects rather than the client objects we might need to keep them registered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to revert this change and we can address the correct place for these in a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, on second thought... doesn't the transport client talk to ES using the wire protocol? why would it need the xcontent bits?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Lets leave this as is then

// ILM - Client - Lifecycle Actions
// ILM
new NamedXContentRegistry.Entry(org.elasticsearch.protocol.xpack.indexlifecycle.LifecycleAction.class,
new ParseField(org.elasticsearch.protocol.xpack.indexlifecycle.AllocateAction.NAME),
org.elasticsearch.protocol.xpack.indexlifecycle.AllocateAction::parse),
Expand Down
14 changes: 7 additions & 7 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@
/**
* Represents the lifecycle of an index from creation to deletion. A
* {@link LifecyclePolicy} is made up of a set of {@link Phase}s which it will
* move through. Soon we will constrain the phases using some kinda of lifecycle
* type which will allow only particular {@link Phase}s to be defined, will
* dictate the order in which the {@link Phase}s are executed and will define
* which {@link LifecycleAction}s are allowed in each phase.
* move through. Policies are constrained by a {@link LifecycleType} which governs which
* {@link Phase}s and {@link LifecycleAction}s are allowed to be defined and in which order
* they are executed.
*/
public class LifecyclePolicy extends AbstractDiffable<LifecyclePolicy>
implements ToXContentObject, Diffable<LifecyclePolicy> {
Expand All @@ -58,9 +57,9 @@ public class LifecyclePolicy extends AbstractDiffable<LifecyclePolicy>
}, PHASES_FIELD);
}

protected final String name;
protected final LifecycleType type;
protected final Map<String, Phase> phases;
private final String name;
private final LifecycleType type;
private final Map<String, Phase> phases;

/**
* @param name
Expand Down Expand Up @@ -97,6 +96,7 @@ public LifecyclePolicy(StreamInput in) throws IOException {
this.type = type;
this.type.validate(phases.values());
}

public static LifecyclePolicy parse(XContentParser parser, String name) {
return PARSER.apply(parser, name);
}
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/Phase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ public static Phase parse(XContentParser parser, String name) {
return PARSER.apply(parser, name);
}

private String name;
private Map<String, LifecycleAction> actions;
private TimeValue after;
private final String name;
private final Map<String, LifecycleAction> actions;
private final TimeValue after;
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch.


/**
* @param name
Expand Down Expand Up @@ -133,12 +133,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.endObject();
return builder;
}

@Override
public int hashCode() {
return Objects.hash(name, after, actions);
}

@Override
public boolean equals(Object obj) {
if (obj == null) {
Expand Down
19 changes: 10 additions & 9 deletions ...re/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/TimeseriesLifecycleType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -46,6 +45,14 @@ public class TimeseriesLifecycleType implements LifecycleType {
static final Set<String> VALID_DELETE_ACTIONS = Sets.newHashSet(ORDERED_VALID_DELETE_ACTIONS);
private static final Phase EMPTY_WARM_PHASE = new Phase("warm", TimeValue.ZERO,
Collections.singletonMap("readonly", ReadOnlyAction.INSTANCE));
private static Map<String, Set<String>> ALLOWED_ACTIONS = new HashMap<>();

static {
ALLOWED_ACTIONS.put("hot", VALID_HOT_ACTIONS);
ALLOWED_ACTIONS.put("warm", VALID_WARM_ACTIONS);
ALLOWED_ACTIONS.put("cold", VALID_COLD_ACTIONS);
ALLOWED_ACTIONS.put("delete", VALID_DELETE_ACTIONS);
}

private TimeseriesLifecycleType() {
}
Expand Down Expand Up @@ -182,18 +189,12 @@ public String getNextActionName(String currentActionName, Phase phase) {

@Override
public void validate(Collection<Phase> phases) {
Set<String> allowedPhases = new HashSet<>(VALID_PHASES);
Map<String, Set<String>> allowedActions = new HashMap<>(allowedPhases.size());
allowedActions.put("hot", VALID_HOT_ACTIONS);
allowedActions.put("warm", VALID_WARM_ACTIONS);
allowedActions.put("cold", VALID_COLD_ACTIONS);
allowedActions.put("delete", VALID_DELETE_ACTIONS);
phases.forEach(phase -> {
if (allowedPhases.contains(phase.getName()) == false) {
if (ALLOWED_ACTIONS.containsKey(phase.getName()) == false) {
throw new IllegalArgumentException("Timeseries lifecycle does not support phase [" + phase.getName() + "]");
}
phase.getActions().forEach((actionName, action) -> {
if (allowedActions.get(phase.getName()).contains(actionName) == false) {
if (ALLOWED_ACTIONS.get(phase.getName()).contains(actionName) == false) {
throw new IllegalArgumentException("invalid action [" + actionName + "] " +
"defined in phase [" + phase.getName() +"]");
}
Expand Down
27 changes: 26 additions & 1 deletion x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.inject.Module;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry.Entry;
Expand All @@ -32,8 +34,17 @@
import org.elasticsearch.watcher.ResourceWatcherService;
import org.elasticsearch.xpack.core.XPackPlugin;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.indexlifecycle.AllocateAction;
import org.elasticsearch.xpack.core.indexlifecycle.DeleteAction;
import org.elasticsearch.xpack.core.indexlifecycle.ForceMergeAction;
import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata;
import org.elasticsearch.xpack.core.indexlifecycle.LifecycleAction;
import org.elasticsearch.xpack.core.indexlifecycle.LifecycleSettings;
import org.elasticsearch.xpack.core.indexlifecycle.LifecycleType;
import org.elasticsearch.xpack.core.indexlifecycle.ReadOnlyAction;
import org.elasticsearch.xpack.core.indexlifecycle.RolloverAction;
import org.elasticsearch.xpack.core.indexlifecycle.ShrinkAction;
import org.elasticsearch.xpack.core.indexlifecycle.TimeseriesLifecycleType;
import org.elasticsearch.xpack.core.indexlifecycle.action.DeleteLifecycleAction;
import org.elasticsearch.xpack.core.indexlifecycle.action.ExplainLifecycleAction;
import org.elasticsearch.xpack.core.indexlifecycle.action.GetLifecycleAction;
Expand Down Expand Up @@ -146,7 +157,21 @@ public List<Entry> getNamedWriteables() {

@Override
public List<org.elasticsearch.common.xcontent.NamedXContentRegistry.Entry> getNamedXContent() {
return Arrays.asList();
return Arrays.asList(
// Custom Metadata
new NamedXContentRegistry.Entry(MetaData.Custom.class, new ParseField(IndexLifecycleMetadata.TYPE),
parser -> IndexLifecycleMetadata.PARSER.parse(parser, null)),
// Lifecycle Types
new NamedXContentRegistry.Entry(LifecycleType.class, new ParseField(TimeseriesLifecycleType.TYPE),
(p, c) -> TimeseriesLifecycleType.INSTANCE),
// Lifecycle Actions
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(AllocateAction.NAME), AllocateAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ForceMergeAction.NAME), ForceMergeAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ReadOnlyAction.NAME), ReadOnlyAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(RolloverAction.NAME), RolloverAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ShrinkAction.NAME), ShrinkAction::parse),
new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(DeleteAction.NAME), DeleteAction::parse)
);
}

@Override
Expand Down
7 changes: 6 additions & 1 deletion ...rotocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/AllocateAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import java.util.Map;
import java.util.Objects;

public class AllocateAction extends LifecycleAction implements ToXContentObject {
public class AllocateAction implements LifecycleAction, ToXContentObject {

public static final String NAME = "allocate";
static final ParseField NUMBER_OF_REPLICAS_FIELD = new ParseField("number_of_replicas");
Expand Down Expand Up @@ -102,6 +102,11 @@ public Map<String, String> getRequire() {
return require;
}

@Override
public String getName() {
return NAME;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
builder.startObject();
Expand Down
7 changes: 6 additions & 1 deletion .../protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/DeleteAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

import java.io.IOException;

public class DeleteAction extends LifecycleAction implements ToXContentObject {
public class DeleteAction implements LifecycleAction, ToXContentObject {
public static final String NAME = "delete";

private static final ObjectParser<DeleteAction, Void> PARSER = new ObjectParser<>(NAME, DeleteAction::new);
Expand All @@ -46,6 +46,11 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par
return builder;
}

@Override
public String getName() {
return NAME;
}

@Override
public int hashCode() {
return 1;
Expand Down
7 changes: 6 additions & 1 deletion ...tocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/ForceMergeAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import java.io.IOException;
import java.util.Objects;

public class ForceMergeAction extends LifecycleAction implements ToXContentObject {
public class ForceMergeAction implements LifecycleAction, ToXContentObject {
public static final String NAME = "forcemerge";
private static final ParseField MAX_NUM_SEGMENTS_FIELD = new ParseField("max_num_segments");

Expand Down Expand Up @@ -60,6 +60,11 @@ public int getMaxNumSegments() {
return maxNumSegments;
}

@Override
public String getName() {
return NAME;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
Expand Down
9 changes: 7 additions & 2 deletions ...otocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/LifecycleAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@
package org.elasticsearch.protocol.xpack.indexlifecycle;

/**
* Marker interface for index lifecycle management actions
* interface for index lifecycle management actions
*/
public class LifecycleAction {
public interface LifecycleAction {

/**
* @return the name of this action
*/
String getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we call this getType() instead? Elsewhere in the codebase we often use name for user specified things and type for types of implementation where the name is fixed. NamedWriteable breaks that a bit for me which bothers me but I think thats the only major exception and isn't user facing

Copy link
Contributor Author

@talevy talevy Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why do you think that type is more appropriate here? The only reason this method exists is for the ability for Phase to leverage its name when looking up NamedXContent. Since this would further diverge the terminology away from our Named- naming world, I feel like calling it type here wouldn't fix much. Also, we have these actions being supported by specific LifecycleTypes (yet another type), that I think there is further confusion to be had there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that being said, I agree that it is unfortunate there is no strong consistency here. For posterity, I'll link to the equivalent info from the Ingest world which uses getType:

elasticsearch/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/GrokProcessor.java

Lines 86 to 89 in cf74898

@Override
public String getType() {
return TYPE;
}

}
Loading