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.

Sign up for GitHub

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

Use a system property to enable inference metadata fields #118876

Merged
merged 4 commits into from
Dec 18, 2024
Merged

Use a system property to enable inference metadata fields #118876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a718a7e
Use a system property to enable inference metadata fields
Mikep86 Dec 17, 2024
af682c7
Merge branch 'inference_metadata_fields' into inference_metadata_fiel…
Mikep86 Dec 17, 2024
0b48f0c
Enable inference metadata fields feature for unit/integration tests o…
Mikep86 Dec 17, 2024
c5ced6e
Merge branch 'inference_metadata_fields' into inference_metadata_fiel…
Mikep86 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/InferenceMetadataFieldsMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@

package org.elasticsearch.index.mapper;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.join.BitSetProducer;
import org.elasticsearch.common.util.FeatureFlag;
import org.elasticsearch.core.Booleans;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.IndexVersions;
import org.elasticsearch.index.query.SearchExecutionContext;
Expand All @@ -26,7 +28,37 @@
* the field name for removal from _source.
*/
public abstract class InferenceMetadataFieldsMapper extends MetadataFieldMapper {
public static final FeatureFlag INFERENCE_METADATA_FIELDS_FEATURE_FLAG = new FeatureFlag("inference_metadata_fields");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - there's no reason not to keep the FF in as well, in case we can remove this system property sooner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add the feature flag back with a one line change if/when we want to re-introduce it. In the meantime,
IMO better to not have a feature flag we know is not being used.

public static class SystemProperty {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to add the possibility of using a FeatureFlag that is false by default, instead of replicating the FF code? 🤔

(I know this is temporary but just wondering)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that would require modifying the FeatureFlag implementation significantly, and I don't want to open that can of worms right now. This is a temporary workaround to allow us to make this feature opt-in without the possibility of affecting other feature flags.

I plan on starting the discussion about opt-in feature flags in the new year, we can address a long-term solution for situations like this at that time.

Copy link
Member

Choose a reason for hiding this comment

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

I understand. I don't like one-off temporary solutions but I get you're trying to get this in for 8.18.

I trust you'll follow up on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will absolutely follow up on this, thank you for understanding the time constraints. And I put the hacky SystemProperty implementation where I did so it will be obvious it needs to be cleaned up once we are no longer using it.

private static final Logger logger = LogManager.getLogger(SystemProperty.class);

private final boolean enabled;

private SystemProperty(String name) {
String propertyName = "es." + name;
this.enabled = parseSystemProperty(propertyName, false);
if (this.enabled) {
logger.info("Feature " + name + " (via system property '" + propertyName + "') is enabled");
} else {
logger.debug("Feature " + name + " (via system property '" + propertyName + "') is disabled");
}
}

private boolean parseSystemProperty(String propertyName, boolean defaultValue) {
final String propertyValue = System.getProperty(propertyName);
logger.trace("System property [{}] is set to [{}]", propertyName, propertyValue);
try {
return Booleans.parseBoolean(propertyValue, defaultValue);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Invalid value [" + propertyValue + "] for system property [" + propertyName + "]", e);
}
}

public boolean isEnabled() {
return enabled;
}
}

public static final SystemProperty INFERENCE_METADATA_FIELDS_SYSTEM_PROPERTY = new SystemProperty("inference_metadata_fields");

public static final String NAME = "_inference_fields";
public static final String CONTENT_TYPE = "_inference_fields";
Expand Down Expand Up @@ -61,6 +93,6 @@ public abstract ValueFetcher valueFetcher(
}

public static boolean isEnabled(IndexVersion indexVersion) {
return indexVersion.onOrAfter(IndexVersions.INFERENCE_METADATA_FIELDS) && INFERENCE_METADATA_FIELDS_FEATURE_FLAG.isEnabled();
return indexVersion.onOrAfter(IndexVersions.INFERENCE_METADATA_FIELDS) && INFERENCE_METADATA_FIELDS_SYSTEM_PROPERTY.isEnabled();
}
}
10 changes: 10 additions & 0 deletions x-pack/plugin/inference/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,17 @@ tasks.named("thirdPartyAudit").configure {
)
}

tasks.named('test') {
if (buildParams.isSnapshotBuild()) {
systemProperty('es.inference_metadata_fields', 'true')
systemProperty('tests.jvm.argline', '-Des.inference_metadata_fields=true')
}
}

tasks.named('yamlRestTest') {
usesDefaultDistribution()
if (buildParams.isSnapshotBuild()) {
systemProperty('tests.jvm.argline', '-Des.inference_metadata_fields=true')
}
}

4 changes: 2 additions & 2 deletions ...nce/src/main/java/org/elasticsearch/xpack/inference/rest/RestGetInferenceModelAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.util.List;
import java.util.Set;

import static org.elasticsearch.index.mapper.InferenceMetadataFieldsMapper.INFERENCE_METADATA_FIELDS_FEATURE_FLAG;
import static org.elasticsearch.index.mapper.InferenceMetadataFieldsMapper.INFERENCE_METADATA_FIELDS_SYSTEM_PROPERTY;
import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.xpack.inference.rest.Paths.INFERENCE_ID;
import static org.elasticsearch.xpack.inference.rest.Paths.INFERENCE_ID_PATH;
Expand Down Expand Up @@ -72,7 +72,7 @@ protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient
public Set<String> supportedCapabilities() {
Set<String> capabilities = new HashSet<>();
capabilities.add(DEFAULT_ELSER_2_CAPABILITY);
if (INFERENCE_METADATA_FIELDS_FEATURE_FLAG.isEnabled()) {
if (INFERENCE_METADATA_FIELDS_SYSTEM_PROPERTY.isEnabled()) {
capabilities.add(INFERENCE_METADATA_FIELDS_CAPABILITY);
}

Expand Down