copy LifecyclePolicy to protocol.xpack by talevy · Pull Request #32915 · elastic/elasticsearch

talevy · 2018-08-16T17:11:40Z

This is the final PR for copying over the necessary components for
clients to parse/render LifecyclePolicy. Changes include:

move of named-x-content server objects away from client
move validation into the client copy of LifecyclePolicy
move LifecycleAction into an interface with getName

This is the final PR for copying over the necessary components for clients to parse/render LifecyclePolicy. Changes include: - move of named-x-content server objects away from client - move validation into the client copy of LifecyclePolicy - move LifecycleAction into an interface with `getName`

dakrone

I left a few comments but nothing major, otherwise LGTM

dakrone · 2018-08-16T21:24:20Z

.../protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/LifecyclePolicy.java

+
+    static void validate(Collection<Phase> phases) {
+        Set<String> allowedPhases = Sets.newHashSet("hot", "warm", "cold", "delete");
+        Map<String, Set<String>> allowedActions = new HashMap<>(allowedPhases.size());


I don't think we need to rebuild this map every time we validate, can this be a static var and initialized in a static block?

dakrone · 2018-08-16T21:24:38Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/Phase.java

+
+    /**
+     * @return a {@link Map} of the {@link LifecycleAction}s to run when during
+     *         his {@link Phase}.


his -> this

this happened somewhere else as well :(

dakrone · 2018-08-16T21:25:14Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/Phase.java

+
+    private String name;
+    private Map<String, LifecycleAction> actions;
+    private TimeValue after;


I think these can all be final?

talevy · 2018-08-16T22:06:38Z

thanks @dakrone! I've updated to reflect your great recommendations!

colings86

I left some comments but it looks good

colings86 · 2018-08-17T11:19:56Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java

-                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),


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?

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

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

Good point. Lets leave this as is then

colings86 · 2018-08-17T11:22:32Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/Phase.java

-    private TimeValue after;
+    private final String name;
+    private final Map<String, LifecycleAction> actions;
+    private final TimeValue after;


good catch.

colings86 · 2018-08-17T11:25:27Z

.../protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/LifecycleAction.java

+    /**
+     * @return the name of this action
+     */
+    String getName();


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

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.

Fair enough

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;

}

colings86 · 2018-08-17T11:45:40Z

.../protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/LifecyclePolicy.java

+        return Strings.toString(this, true, true);
+    }
+
+    static void validate(Collection<Phase> phases) {


I think we can make this private? It shouldn't be too hard to test this method via the constructor?

for sure! I'll just get rid of this method entirely

colings86 · 2018-08-17T11:46:08Z

.../protocol/src/main/java/org/elasticsearch/protocol/xpack/indexlifecycle/LifecyclePolicy.java

+    public LifecyclePolicy(String name, Map<String, Phase> phases) {
+        this.name = name;
+        this.phases = phases;
+        validate(phases.values());


super nit: I tend to like validation to be first

will push up!

colings86

LGTM

talevy · 2018-08-20T15:32:37Z

thanks for the review @colings86 and @dakrone!

This is the final PR for copying over the necessary components for clients to parse/render LifecyclePolicy. Changes include: - move of named-x-content server objects away from client - move validation into the client copy of LifecyclePolicy - move LifecycleAction into an interface with `getName`

talevy added WIP :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. labels Aug 16, 2018

elasticmachine mentioned this pull request Aug 16, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

talevy force-pushed the ilm-protocol-policy branch 3 times, most recently from 6420f6d to 8550d71 Compare August 16, 2018 17:23

talevy force-pushed the ilm-protocol-policy branch from 8550d71 to a6bb033 Compare August 16, 2018 21:11

talevy requested review from colings86 and dakrone August 16, 2018 21:11

talevy removed the WIP label Aug 16, 2018

dakrone approved these changes Aug 16, 2018

View reviewed changes

respond to review comments

a2f675f

Merge branch 'index-lifecycle' into ilm-protocol-policy

7d76855

colings86 suggested changes Aug 17, 2018

View reviewed changes

talevy added 2 commits August 17, 2018 14:04

Merge branch 'index-lifecycle' into ilm-protocol-policy

880e3f9

move validate inline

8c764b4

colings86 approved these changes Aug 20, 2018

View reviewed changes

talevy merged commit 5ce082c into elastic:index-lifecycle Aug 20, 2018

talevy deleted the ilm-protocol-policy branch August 20, 2018 15:32

Conversation

talevy commented Aug 16, 2018

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

talevy commented Aug 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

talevy commented Aug 16, 2018 •

edited

Loading

talevy Aug 17, 2018 •

edited

Loading