Skip to content

Conversation

@jbaiera
Copy link
Member

@jbaiera jbaiera commented Jul 18, 2019

This PR adds version information to an EnrichPolicy when it is created to aid with backwards compatibility logic. The old EnrichPolicy object has been renamed to EnrichPolicyDefinition and a new EnrichPolicy object has been created, one that holds the version of Elasticsearch that the policy was created on. I also added the name to the policy object since in many places we are passing the name around along with the policy definition. All response objects have been updated to return the new policy structure. Now when a get operation is performed, the following is returned:

{
  "name": "policy-name",
  "version_created": "8.0.0",
  "definition": {
    "type": "exact_match",
    "indices": ["source_index"],
    "query": { ... },
    "enrich_key": "keyname",
    "enrich_values": ["field1", "field2", "field3"]
  }
}

@jbaiera jbaiera added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Jul 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

parser.declareString(ConstructingObjectParser.constructorArg(), ENRICH_KEY);
parser.declareStringArray(ConstructingObjectParser.constructorArg(), ENRICH_VALUES);
PARSER.declareString(ConstructingObjectParser.constructorArg(), NAME);
PARSER.declareField(ConstructingObjectParser.constructorArg(), (p, c) -> Version.fromString(p.text()), VERSION_CREATED,
Copy link
Contributor

@jakelandis jakelandis Jul 23, 2019

Choose a reason for hiding this comment

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

Can newer ES versions create a Version object (via the fromString) that is not explicitly defined. e.g. what happens if version 6.8.0 is the version serialized can this read it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - The fromString() method condenses the version string given down into the integral version id. If the version id isn't defined in the known list of versions, it is still created, but the version of lucene that it associates with the returned version is a guess. I don't foresee this being an issue, but I could be off base here.


public Response(Map<String, EnrichPolicy> policies) {
Objects.requireNonNull(policies, "policies cannot be null");
// use a treemap to guarantee ordering in the set, then transform it to the list of named policies
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the comment, this no longer uses a Treemap

builder.field(NAME.getPreferredName(), name);
builder.field(VERSION_CREATED.getPreferredName());
versionCreated.toXContent(builder, params);
builder.startObject(DEFINITION.getPreferredName());
Copy link
Contributor

Choose a reason for hiding this comment

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

does the definition need be a sub object ?

- match: { enrich_key: baz }
- match: { enrich_values: ["a", "b"] }
- match: { name: policy-crud }
- match: { definition.type: exact_match }
Copy link
Contributor

Choose a reason for hiding this comment

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

asked above ^^ if can avoid the definition sub object .

@jakelandis
Copy link
Contributor

Is it possible to keep this as single policy object which can optionally serialize/deserialize the extra two fields ?

@jbaiera
Copy link
Member Author

jbaiera commented Jul 24, 2019

Is it possible to keep this as single policy object which can optionally serialize/deserialize the extra two fields ?

I wanted to avoid having fields on the policy object that would need to be handled specially for the purposes of serialization. More specifically, a user shouldn't be able to set the version_created field on a policy, but the policy would still need to be able to read/write the field as JSON for the purpose of existing in the cluster state. I wasn't sure if there was a way of saying "this field isn't allowed to be set by the user, but should be allowed to be read from the cluster state" without some awkward code along side it with the caveats mentioned. The separated object just felt a little cleaner.

@martijnvg
Copy link
Member

Maybe introduce a second map in EnrichMetadata that contains a new class with the index version instead of a new class that wraps a policy?

@jbaiera
Copy link
Member Author

jbaiera commented Jul 30, 2019

Closing this out in favor of #45021 - It accomplishes the same thing, but with less ripple effect.

@jbaiera jbaiera closed this Jul 30, 2019
@jbaiera jbaiera deleted the enrich-add-policy-metadata branch July 30, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants