Add the ability to dynamically update similarity options

[jimczi]

This change allows to dynamically change options for any existing similarities on an open index.
This change also adds the ability to create a new similarity directly on an existing index.
It does not allow to change the type of an existing similarity or to change the similarity of a field.
Options are updatable only if they don't change the way the similarity encodes the norm on disk.
The non-updatable options can't be changed if the index is opened nor closed.
This change adds validation to the similarity settings and throws an exception with a detailed message if a setting is unknown.

Relates #6727

[clintongormley]

is this breaking? i think it's just an enhancement, no?

[jimczi]

@clintongormley I had to change the SimilarityProvider interface which can be used in a plugin so I marked this as breaking ? It is an enhancement though but it's also a breaking change for users that have a custom similarity plugin.

[rmuir]

Yes this logic looks right at a glance: disallow changing discountOverlaps as it changes the index-time encoding of the norm value.

NOTE: for all the lucene similarities, the index-time encoding is the same (provided discountOverlaps is the same). so changing between them could be allowed, too.

But its definitely important ppl can change stuff like bm25 parameters.

[jimczi]

NOTE: for all the lucene similarities, the index-time encoding is the same (provided discountOverlaps is the same). so changing between them could be allowed, too.

Thanks @rmuir, I want to do this in a follow up but the first step is to be able to change the similarity options.

[rmuir]

Thanks @rmuir, I want to do this in a follow up but the first step is to be able to change the similarity options.

+1

My only suggestion for this one then, is to improve the exception if you try to change discount_overlaps:
throw new IllegalArgumentException("");.

[jimczi]

@s1monw I have a dynamic setting with a custom validator for the update:

AbstractScopedSettings.addSettingsUpdateConsumer(Setting<T> setting, Consumer<T> consumer, Consumer<T> validator)

My validator throws RuntimeException if the setting update is not valid. I've added a test that update my setting with invalid values:
https://github.com/jimferenczi/elasticsearch/blob/4829959be53ec19f988532e9d74a7ce6c77dba07/core/src/test/java/org/elasticsearch/index/similarity/SimilarityTests.java#L91
... but it fails. The exception is logged, the settings are applied and the request returns 200 OK.
This looks like a bug but I am not sure that I am using the validator correctly.
Could you please take a quick look at:
https://github.com/jimferenczi/elasticsearch/blob/4829959be53ec19f988532e9d74a7ce6c77dba07/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java#L40
and
https://github.com/jimferenczi/elasticsearch/blob/4829959be53ec19f988532e9d74a7ce6c77dba07/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java#L119
?

[s1monw]

@jimferenczi I just added a unittest to ensure it works but it seems fine, can you point me to the test that fails?

[jimczi]

@s1monw thank you for checking. Here is a quick and dirty IT that should fail:
master...jimferenczi:dynamic_setting

It does not fail because the setting update is tested on the master with fake index scopped setting:

elasticsearch/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java

Line 65 in 42526ac

private final IndexScopedSettings indexScopedSettings;

We could retrieve the setting from the index directly but the index may not exist on the master node.
To circumvent this problem the mapping service creates/removes the index:
https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java#L141
... but it seems overkill to do the same for every settings update ?

[s1monw]

@jimferenczi I opened #19088 for the settings problems

[jimczi]

Now that #19088 and #19122 are merged the tests are ok and this change is ready for review.

[jimczi]

Pushed another iteration. I updated the description with the current state.
@s1monw can you take a look when you have time ?

[jpountz]

does it really work to register a prefix? If yes, can you leave a comment, I think that would help since none of the other settings are registered this way

[jpountz]

This change has a lot to do with settings, which are a bit out of my comfort zone, but the Similarity part looks good to me. I'd suggest to give another day to give Simon a chance to have a last look before merging.

[s1monw]

I think what we should do here is to add functionality to specify something like this:

Instead of private static final Setting<Float> LAMBDA_SETTING = Setting.floatSetting("lambda", 0.1f, Setting.Property.Dynamic); we would add private static final Setting<Float> LAMBDA_SETTING = Setting.floatSetting("index.similarity.*.lambda", 0.1f, Setting.Property.Dynamic); and add the ability for making this setting concrete via something like Setting#concretSetting(name). For the updateConsumer API I guess we must add another method that accepts the generic setting and the concrete one to pass the checks in there? @jimferenczi what do you think?

[jimczi]

Not sure how this would work. The concrete setting name and the acceptable inner settings can be resolved only when the user register a new similarity with a type like in:
index.similarity.my_similarity.type: BM25
This definition makes the settings index.similarity.my_similarity.band index.similarity.my_similarity.k1 acceptable.
Are you saying that we should call Setting#concreteSetting(name) for every custom similarity that the user defines ? We would have to hack this a bit since the similarity type is the setting that would unlock the other possible settings.

[jimczi]

superseded by #20339 which uses the settings framework and the affix settings to achieve similar behavior.