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

[Transform] Add _meta field to TransformConfig #79003

Merged
merged 6 commits into from
Oct 15, 2021

Conversation

przemekwitek
Copy link
Contributor

This PR adds a new field (called _meta) to the TransformConfig.
This _meta field stores an arbitrary key-value map.
Keys are strings.
Values are arbitrary objects (possibly also maps).
The _meta map is updatable via _update endpoint in Transform API.

Relates #77506

@przemekwitek przemekwitek removed the WIP label Oct 12, 2021
@przemekwitek przemekwitek marked this pull request as ready for review October 12, 2021 16:21
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Oct 12, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

some comments:

  • we should add a mapping(FLATTENED) to TransformInternalIndex, this is not strictly required as dynamic mappings are turned off, but we do the same for other fields -> consistency
  • it would be nice to not allow arbitrary sized meta entries, the other parts of the config are somewhat limited by nature (assuming no one is writing a monster aggregation), but meta is unlimited
    • however that's hard to achieve with the current parser
    • we could as alternative limit the Rest Request (RestPutTransformAction)
  • +1 regarding the idea of using LinkedHashMap to preserve order

Copy link
Contributor Author

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

some comments:

we should add a mapping(FLATTENED) to TransformInternalIndex, this is not strictly required as dynamic mappings are
turned off, but we do the same for other fields -> consistency

Done.

it would be nice to not allow arbitrary sized meta entries, the other parts of the config are somewhat limited by nature
(assuming no one is writing a monster aggregation), but meta is unlimited
however that's hard to achieve with the current parser
we could as alternative limit the Rest Request (RestPutTransformAction)

And also RestUpdateTransformAction, right?
What limit do you think makes sense here? Any prior art?

+1 regarding the idea of using LinkedHashMap to preserve order

Done.

@@ -173,6 +189,9 @@ public void writeTo(final StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.V_7_8_0)) {
out.writeOptionalWriteable(settings);
}
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
out.writeMapWithConsistentOrder(metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this to cause an assertion failure when used in conjunction with mapOrdered if we have good test coverage:

assert false == (map instanceof LinkedHashMap);

For a linked hash map just writeMap should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, I think this is probably fine, it iterates the keys and since LinkedHashMap has consistent order, that will work.

On reading though, this means that you cannot use readMap, you will need to readOrderedMap or whatever.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM, if you could just fix a couple of field names in the docs

docs/reference/transform/apis/update-transform.asciidoc Outdated Show resolved Hide resolved
docs/reference/transform/apis/put-transform.asciidoc Outdated Show resolved Hide resolved
@hendrikmuhs
Copy link

And also RestUpdateTransformAction, right?
What limit do you think makes sense here? Any prior art?

no prior art as far as I know.

I think if we limit to 5MB we still allow a lot of room for super large in-config painless scripts.

Note, I am not concerned about a single put or update request. The real issue is getting the configs in some internal places where we sometimes retrieve 100, sometimes up to 10k at a time.

@przemekwitek
Copy link
Contributor Author

run elasticsearch-ci/part-2

@przemekwitek
Copy link
Contributor Author

I think if we limit to 5MB we still allow a lot of room for super large in-config painless scripts.

Ok. I've added a limit of 5MB to both Put and Update. I've manually verified (by temporarily lowering the limit) that an exception when the request is too large.

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

@przemekwitek przemekwitek merged commit 1595d3a into elastic:master Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants