-
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
Introduce data stream options and failure store configuration classes #109515
Changes from 8 commits
da3bab3
6352d2e
9e41720
9f19c4a
47cca13
4fcd3af
89c950b
9b1984e
fbc03be
2efb558
3fcf60c
a0453dc
dcc04fd
28777ee
8613f63
24aa320
dd769d0
9f1fd8c
a26bb6d
f31a50a
dc4cb37
1d3d566
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.cluster.metadata; | ||
|
||
import org.elasticsearch.cluster.Diff; | ||
import org.elasticsearch.cluster.SimpleDiffable; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.core.Nullable; | ||
import org.elasticsearch.xcontent.ConstructingObjectParser; | ||
import org.elasticsearch.xcontent.ParseField; | ||
import org.elasticsearch.xcontent.ToXContentObject; | ||
import org.elasticsearch.xcontent.XContentBuilder; | ||
import org.elasticsearch.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
|
||
/** | ||
* Holds the data stream failure store metadata that enable or disable the failure store of a data stream. Currently, it | ||
* supports the following configurations: | ||
* - enabled | ||
*/ | ||
public record DataStreamFailureStore(boolean enabled) implements SimpleDiffable<DataStreamFailureStore>, ToXContentObject { | ||
|
||
public static final ParseField ENABLED_FIELD = new ParseField("enabled"); | ||
|
||
public static final ConstructingObjectParser<DataStreamFailureStore, Void> PARSER = new ConstructingObjectParser<>( | ||
"failure_store", | ||
false, | ||
(args, unused) -> new DataStreamFailureStore((Boolean) args[0]) | ||
); | ||
|
||
static { | ||
PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), ENABLED_FIELD); | ||
} | ||
|
||
public DataStreamFailureStore() { | ||
this(true); | ||
} | ||
nielsbauman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public DataStreamFailureStore(@Nullable Boolean enabled) { | ||
this(enabled == null || enabled); | ||
} | ||
gmarouli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public static DataStreamFailureStore read(StreamInput in) throws IOException { | ||
return new DataStreamFailureStore(in.readBoolean()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you went with a static method rather than a constructor? We've been moving to constructors for this more and more in the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I wasn't aware we prefer constructors. I preferred the static method because it allows more flexibility, so I thought it was better to always use a static method to facilitate all cases. However, if this is not the direction we are following and in this case there is no blocker in using a constructor, I will change it to use a constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm personally also more of a fan of static methods (because in some cases we need to use a static method rather than a constructor). |
||
|
||
public static Diff<DataStreamFailureStore> readDiffFrom(StreamInput in) throws IOException { | ||
return SimpleDiffable.readDiffFrom(DataStreamFailureStore::read, in); | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
out.writeBoolean(enabled); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return Strings.toString(this, true, true); | ||
} | ||
|
||
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(); | ||
builder.field(ENABLED_FIELD.getPreferredName(), enabled); | ||
builder.endObject(); | ||
return builder; | ||
} | ||
|
||
public static DataStreamFailureStore fromXContent(XContentParser parser) throws IOException { | ||
return PARSER.parse(parser, null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.cluster.metadata; | ||
|
||
import org.elasticsearch.cluster.Diff; | ||
import org.elasticsearch.cluster.SimpleDiffable; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.core.Nullable; | ||
import org.elasticsearch.xcontent.ConstructingObjectParser; | ||
import org.elasticsearch.xcontent.ObjectParser; | ||
import org.elasticsearch.xcontent.ParseField; | ||
import org.elasticsearch.xcontent.ToXContentObject; | ||
import org.elasticsearch.xcontent.XContentBuilder; | ||
import org.elasticsearch.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
|
||
/** | ||
* Holds data stream dedicated configuration options such as failure store, (in the future lifecycle). Currently, it | ||
* supports the following configurations: | ||
* - lifecycle | ||
* - failure store | ||
*/ | ||
public record DataStreamOptions(@Nullable DataStreamLifecycle lifecycle, @Nullable DataStreamFailureStore failureStore) | ||
implements | ||
SimpleDiffable<DataStreamOptions>, | ||
ToXContentObject { | ||
|
||
public static final ParseField LIFECYCLE_FIELD = new ParseField("lifecycle"); | ||
public static final ParseField FAILURE_STORE_FIELD = new ParseField("failure_store"); | ||
|
||
public static final ConstructingObjectParser<DataStreamOptions, Void> PARSER = new ConstructingObjectParser<>( | ||
"options", | ||
false, | ||
(args, unused) -> new DataStreamOptions((DataStreamLifecycle) args[0], (DataStreamFailureStore) args[1]) | ||
); | ||
|
||
static { | ||
PARSER.declareField( | ||
ConstructingObjectParser.optionalConstructorArg(), | ||
(p, c) -> DataStreamLifecycle.fromXContent(p), | ||
LIFECYCLE_FIELD, | ||
ObjectParser.ValueType.OBJECT_OR_NULL | ||
); | ||
PARSER.declareField( | ||
ConstructingObjectParser.optionalConstructorArg(), | ||
(p, c) -> DataStreamFailureStore.fromXContent(p), | ||
FAILURE_STORE_FIELD, | ||
ObjectParser.ValueType.OBJECT_OR_NULL | ||
); | ||
} | ||
|
||
public DataStreamOptions() { | ||
this(null, null); | ||
} | ||
|
||
public static DataStreamOptions read(StreamInput in) throws IOException { | ||
return new DataStreamOptions( | ||
in.readOptionalWriteable(DataStreamLifecycle::new), | ||
in.readOptionalWriteable(DataStreamFailureStore::read) | ||
); | ||
} | ||
|
||
public static Diff<DataStreamOptions> readDiffFrom(StreamInput in) throws IOException { | ||
return SimpleDiffable.readDiffFrom(DataStreamOptions::read, in); | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
out.writeOptionalWriteable(lifecycle); | ||
out.writeOptionalWriteable(failureStore); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return Strings.toString(this, true, true); | ||
} | ||
|
||
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(); | ||
if (lifecycle != null) { | ||
builder.field(LIFECYCLE_FIELD.getPreferredName()); | ||
builder.value(lifecycle); | ||
} | ||
if (failureStore != null) { | ||
builder.field(FAILURE_STORE_FIELD.getPreferredName()); | ||
builder.value(failureStore); | ||
gmarouli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
builder.endObject(); | ||
return builder; | ||
} | ||
|
||
public static DataStreamOptions fromXContent(XContentParser parser) throws IOException { | ||
return PARSER.parse(parser, null); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.cluster.metadata; | ||
|
||
import org.elasticsearch.common.io.stream.Writeable; | ||
import org.elasticsearch.test.AbstractXContentSerializingTestCase; | ||
import org.elasticsearch.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
|
||
public class DataStreamFailureStoreTests extends AbstractXContentSerializingTestCase<DataStreamFailureStore> { | ||
|
||
@Override | ||
protected Writeable.Reader<DataStreamFailureStore> instanceReader() { | ||
return DataStreamFailureStore::read; | ||
} | ||
|
||
@Override | ||
protected DataStreamFailureStore createTestInstance() { | ||
return randomFailureStore(); | ||
} | ||
|
||
@Override | ||
protected DataStreamFailureStore mutateInstance(DataStreamFailureStore instance) throws IOException { | ||
return new DataStreamFailureStore(instance.enabled() == false); | ||
} | ||
|
||
@Override | ||
protected DataStreamFailureStore doParseInstance(XContentParser parser) throws IOException { | ||
return DataStreamFailureStore.fromXContent(parser); | ||
} | ||
|
||
static DataStreamFailureStore randomFailureStore() { | ||
return new DataStreamFailureStore(randomBoolean() ? null : randomBoolean()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.cluster.metadata; | ||
|
||
import org.elasticsearch.common.io.stream.Writeable; | ||
import org.elasticsearch.test.AbstractXContentSerializingTestCase; | ||
import org.elasticsearch.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
|
||
public class DataStreamOptionsTests extends AbstractXContentSerializingTestCase<DataStreamOptions> { | ||
|
||
@Override | ||
protected Writeable.Reader<DataStreamOptions> instanceReader() { | ||
return DataStreamOptions::read; | ||
} | ||
|
||
@Override | ||
protected DataStreamOptions createTestInstance() { | ||
return new DataStreamOptions( | ||
randomBoolean() ? null : DataStreamLifecycleTests.randomLifecycle(), | ||
randomBoolean() ? null : DataStreamFailureStoreTests.randomFailureStore() | ||
); | ||
} | ||
|
||
@Override | ||
protected DataStreamOptions mutateInstance(DataStreamOptions instance) throws IOException { | ||
var lifecycle = instance.lifecycle(); | ||
var failureStore = instance.failureStore(); | ||
if (randomBoolean()) { | ||
if (lifecycle == null) { | ||
lifecycle = DataStreamLifecycleTests.randomLifecycle(); | ||
} else { | ||
lifecycle = randomBoolean() ? null : randomValueOtherThan(lifecycle, DataStreamLifecycleTests::randomLifecycle); | ||
} | ||
} else { | ||
if (failureStore == null) { | ||
failureStore = DataStreamFailureStoreTests.randomFailureStore(); | ||
} else { | ||
failureStore = randomBoolean() ? null : new DataStreamFailureStore(failureStore.enabled() == false); | ||
} | ||
} | ||
return new DataStreamOptions(lifecycle, failureStore); | ||
} | ||
|
||
@Override | ||
protected DataStreamOptions doParseInstance(XContentParser parser) throws IOException { | ||
return DataStreamOptions.fromXContent(parser); | ||
} | ||
} |
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.
One of the things we're talking about doing is adding a cluster setting that will contain index patterns which will state that failure store is enabled for any matching indices by default. The plan is to fall back to using that cluster setting in the case that failure stores haven't been explicitly enabled or disabled via templates at creation time, or in the state.
To save on headache going forward, should this boolean be an optional
Boolean
?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.
Good point! thinking further about this, I would expect that the following to be the not explicitly set:
This follows the way that the lifecycle works. In template composition, a user can set
failure_store: null
to remove any configuration set by previous templates.Since this is a new configuration type we could always decide we would like to change the way we work with this, but I thought we should at least consider it.
What do you think?
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.
Ah I see, since there is only one property we can just assume the entire section will be removed from options if it's not present. I'm good with that