-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[ML][Data Frame] Add version and create_time to transform config #43384
[ML][Data Frame] Add version and create_time to transform config #43384
Conversation
Pinging @elastic/ml-core |
@@ -40,6 +44,8 @@ | |||
public static final ParseField SOURCE = new ParseField("source"); | |||
public static final ParseField DEST = new ParseField("dest"); | |||
public static final ParseField DESCRIPTION = new ParseField("description"); | |||
public static final ParseField TRANSFORM_VERSION = new ParseField("transform_version"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should call this just version
. We went for job_version
for anomaly detection jobs in hindsight it seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I am fine with changing it.
@@ -48,6 +54,8 @@ | |||
private final DestConfig dest; | |||
private final PivotConfig pivotConfig; | |||
private final String description; | |||
private final Version transformVersion; | |||
private final Date createTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is new code, should we use the new java time classes (I guess Instant
in this case)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimitris-athanasiou is this a thing? I did not know we should be using Instant instead of Date and that one supplanted the other. TIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, use Instant
and serialize it using StreamOutput.writeInstant()
and StreamInput.readInstant()
(or StreamOutput.writeOptionalInstant()
and StreamInput.readOptionalInstant()
if more appropriate).
run elasticsearch-ci/bwc |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…stic#43384) * [ML][Data Frame] Add version and create_time to transform config * s/transform_version/version s/Date/Instant * fixing getter/setter for version
This adds two fields to the data frame transform config:
transform_version
: indicates in which version of ES the transform was createdcreate_time
: indicates the time at which the transform was createBoth are available in the response payloads, and are only added on PUT. Logic is in place to verify that the user cannot provide either of these fields on creation.
closes #43037