[ML] Add internal flag to flush api to control whether or not to flush indices #96803

edsavage · 2023-06-13T15:01:06Z

When datafeeds send flush requests they don't require the indices to be refreshed (and in fact may be detrimentally expensive in the case of small bucket sizes).

This change adds an (internal) flag to control whether or not a refresh is required when flushing.

…dices or not. When datafeeds send flush requests they don't require the indices to be refreshed. First working version.

* change naming * remove unnecessary logging

…d_internal_flag_to_flush_api

… fix Always refresh indices in FlushAcknowlegement instance created within integration tests. This is to ensure consistent behaviour across test runs.

…d_internal_flag_to_flush_api

edsavage · 2023-06-14T14:17:00Z

retest

…d_internal_flag_to_flush_api

elasticsearchmachine · 2023-06-14T15:11:19Z

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

droberts195 · 2023-06-14T15:55:34Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java


    private final String id;
    private final Instant lastFinalizedBucketEnd;
+    private final Boolean refreshRequired;


I think this should be boolean, and always set, because the constructor sets it to refreshRequired == null || refreshRequired.

droberts195 · 2023-06-14T15:56:31Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java

    public FlushAcknowledgement(StreamInput in) throws IOException {
        id = in.readString();
        lastFinalizedBucketEnd = in.readOptionalInstant();
+        refreshRequired = in.readOptionalBoolean();


Suggested change

refreshRequired = in.readOptionalBoolean();

refreshRequired = in.readBoolean();

Also, needs BWC protection.

droberts195 · 2023-06-14T15:56:46Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java

    public void writeTo(StreamOutput out) throws IOException {
        out.writeString(id);
        out.writeOptionalInstant(lastFinalizedBucketEnd);
+        out.writeOptionalBoolean(refreshRequired);


Suggested change

out.writeOptionalBoolean(refreshRequired);

out.writeBoolean(refreshRequired);

And BWC needs to be handled too.

droberts195 · 2023-06-14T15:57:48Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java

        return lastFinalizedBucketEnd;
    }

+    public Boolean getRefreshRequired() {


Suggested change

public Boolean getRefreshRequired() {

public boolean getRefreshRequired() {

droberts195 · 2023-06-14T15:58:18Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java

-        return Objects.equals(id, other.id) && Objects.equals(lastFinalizedBucketEnd, other.lastFinalizedBucketEnd);
+        return Objects.equals(id, other.id)
+            && Objects.equals(lastFinalizedBucketEnd, other.lastFinalizedBucketEnd)
+            && Objects.equals(refreshRequired, other.refreshRequired);


This can be a simple comparison after switching to boolean.

droberts195 · 2023-06-14T16:01:18Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/FlushJobAction.java

            out.writeOptionalString(advanceTime);
            out.writeOptionalString(skipTime);
            out.writeBoolean(waitForNormalization);
+            out.writeBoolean(refreshRequired);


This needs BWC protection. Please read the JavaDocs in TransportVersion.java to see how to do it now.

droberts195 · 2023-06-14T16:01:37Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/FlushJobAction.java

            advanceTime = in.readOptionalString();
            skipTime = in.readOptionalString();
            waitForNormalization = in.readBoolean();
+            refreshRequired = in.readBoolean();


Needs BWC protection.

droberts195 · 2023-06-14T16:18:43Z

...l/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/params/FlushJobParams.java

        return waitForNormalization;
    }

+    public Boolean refreshRequired() {


Suggested change

public Boolean refreshRequired() {

public boolean refreshRequired() {

* use boolean in preference to Boolean * BWC guards

droberts195 · 2023-06-15T10:08:30Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/FlushJobAction.java

            PARSER.declareString(Request::setEnd, END);
            PARSER.declareString(Request::setAdvanceTime, ADVANCE_TIME);
            PARSER.declareString(Request::setSkipTime, SKIP_TIME);
+            PARSER.declareBoolean(Request::setRefreshRequired, REFRESH_REQUIRED);


Just thinking about this a bit more, we said we wanted this to be internal-only, so it shouldn't be available in the JSON parser. (Like waitForNormalization isn't in this block.)

droberts195 · 2023-06-15T10:10:04Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/FlushJobAction.java

            if (skipTime != null) {
                builder.field(SKIP_TIME.getPreferredName(), skipTime);
            }
+            builder.field(REFRESH_REQUIRED.getPreferredName(), refreshRequired);


Similarly, it shouldn't be included in the JSON representation. (Like waitForNormalization is not mentioned in this method.)

droberts195 · 2023-06-15T10:10:46Z

...java/org/elasticsearch/xpack/core/ml/job/process/autodetect/output/FlushAcknowledgement.java

    static {
        PARSER.declareString(ConstructingObjectParser.constructorArg(), ID);
        PARSER.declareLong(ConstructingObjectParser.optionalConstructorArg(), LAST_FINALIZED_BUCKET_END);
+        PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), REFRESH_REQUIRED);


Here it is correct to include the internal field in the parser, as this is for internal communications with our C++ code.

* Do not parse "refreshRequired" flag in FlushJobAction

* For backwards compatibility it must be possible to construct FlushAcknowledgement without refreshRequired, i.e. refreshRequired must be nullable. * Added missing comparisons in FlushParams equals and hashCode methods.

droberts195

LGTM

…d_internal_flag_to_flush_api

For transport versions prior to V_8_500_012 refreshRequired in FlushJobAction.Request is always set to true. Update the tests to reflect that.

edsavage added 2 commits June 13, 2023 14:51

[ML] Add internal flag to flush api determining whether to refresh in…

b14c392

…dices or not. When datafeeds send flush requests they don't require the indices to be refreshed. First working version.

Tidy up

1a781b6

* change naming * remove unnecessary logging

edsavage added :ml Machine learning v8.9.0 labels Jun 13, 2023

edsavage added 6 commits June 13, 2023 16:37

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

0d4b8cf

…d_internal_flag_to_flush_api

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

c41f7ff

…d_internal_flag_to_flush_api

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

f624514

…d_internal_flag_to_flush_api

AutodetectResultProcessorIT.testMultipleFlushesBetweenPersisting test…

c41061b

… fix Always refresh indices in FlushAcknowlegement instance created within integration tests. This is to ensure consistent behaviour across test runs.

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

5b47c56

…d_internal_flag_to_flush_api

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

515a9ff

…d_internal_flag_to_flush_api

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

0ea6cf2

…d_internal_flag_to_flush_api

edsavage marked this pull request as ready for review June 14, 2023 15:10

elasticsearchmachine added the Team:ML Meta label for the ML team label Jun 14, 2023

droberts195 added the >non-issue label Jun 14, 2023

edsavage added >enhancement and removed >enhancement labels Jun 14, 2023

droberts195 reviewed Jun 14, 2023

View reviewed changes

Attend to review comments

c276fc4

* use boolean in preference to Boolean * BWC guards

droberts195 reviewed Jun 15, 2023

View reviewed changes

edsavage added 2 commits June 15, 2023 11:23

Attend to further review comments

304f30e

* Do not parse "refreshRequired" flag in FlushJobAction

Fix failing BWC tests

583a440

* For backwards compatibility it must be possible to construct FlushAcknowledgement without refreshRequired, i.e. refreshRequired must be nullable. * Added missing comparisons in FlushParams equals and hashCode methods.

droberts195 approved these changes Jun 15, 2023

View reviewed changes

edsavage added 3 commits June 15, 2023 14:26

Merge branch 'main' of github.com:elasticsearch/elasticsearch into ad…

c157f74

…d_internal_flag_to_flush_api

Fix failing BWC test

212ca28

For transport versions prior to V_8_500_012 refreshRequired in FlushJobAction.Request is always set to true. Update the tests to reflect that.

Fix format violation

d3c6cc3

edsavage merged commit cbe8aa9 into elastic:main Jun 15, 2023

edsavage deleted the add_internal_flag_to_flush_api branch June 15, 2023 15:33

	refreshRequired = in.readOptionalBoolean();
	refreshRequired = in.readBoolean();

	out.writeOptionalBoolean(refreshRequired);
	out.writeBoolean(refreshRequired);

	public Boolean getRefreshRequired() {
	public boolean getRefreshRequired() {

	public Boolean refreshRequired() {
	public boolean refreshRequired() {

[ML] Add internal flag to flush api to control whether or not to flush indices #96803

[ML] Add internal flag to flush api to control whether or not to flush indices #96803

Uh oh!

Conversation

edsavage commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edsavage commented Jun 14, 2023

Uh oh!

elasticsearchmachine commented Jun 14, 2023

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!

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!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edsavage commented Jun 13, 2023 •

edited

Loading