-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Introduce Write Indices To Aliases #30703
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
Conversation
This commit introduces the concept of an `is_write_index` flag for aliases. Before, it was invalid to write to indices which point to multiple indices. Now, users can configure one index to have an alias with `is_write_index: true` and others that are false. When one attempts to index to this alias, it will resolve to the one index that is the write index. in response to elastic#30061. This PR rebuilds aliasAndIndexLookup every time it wants to use it for alias validation. Depends on outcome of elastic#29575 for using a pre-built data-structure. This PR introduces a concept in aliases called `is_write_index`. When `true` on a specific alias definition associated with an `index`, index requests using the associated alias name will resolve to this `index` for indexing. Write now, if one attempts to index a document using an alias that points to multiple indices, there is no way to resolve which one, so an exception is thrown. With this flag, you can have an alias that points to a specific index in a set of indices as the one to write to. It is possible to set an index as an alias' write-index during index creation: ``` PUT foo1 { "aliases": { "bar": { "is_write_index": true } } } ``` Now, index requests like `PUT bar/_doc/_id` will route to index `foo1`. Another index, `foo2`, is free to also have an alias named "bar" pointing to it. This alias needs to be configured with `is_write_index` set to `false`. `is_write_index` is defaulted to `false` if not specified, or it can be explicitly set to `false`. ``` PUT foo2 { "aliases": { "bar": {} } } ``` Now, searches against `bar` will point to both indices, but index requests will be routed to `foo1`. These settings can be updated in the `_aliases` API like so: ``` POST _aliases { "actions": [ { "add": { "index": "foo1", "alias": "bar", "is_write_index": false } } ] } ``` How does this relate to Rollover? Rollover depended on rollover-aliases that only point to one index since multiple would break indexing. Now that the same alias can point to multiple indices, the only requirement we have is that there is only one write index. Here is an example scenario that didn't work before, but works with these changes: ``` PUT _template/my_template { "index_patterns": ["foo-*"], "settings": { "number_of_shards": 1, "number_of_replicas": 1 }, "aliases": { "logs": {} }, "mappings": { "_doc": { "_source": { "enabled": false } } } } PUT foo-000001 { "aliases": { "logs": { "is_write_index": true } } } PUT logs/_doc/_id { "hello": "world" } POST /logs/_rollover/ { "conditions": { "max_docs": 1 } } GET foo-000001 # now this is `is_write_index: false` GET foo-000002 # now this is `is_write_index: true` ```
Pinging @elastic/es-core-infra |
if (Version.CURRENT.onOrAfter(Version.V_6_4_0)) { | ||
return IndicesOptions.strictAliasToWriteIndexNoExpandForbidClosed(); | ||
} | ||
return super.indicesOptions(); |
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.
Wy do we need this if? Seems like we could rather push this change to the proper branches without this conditional?
@Override | ||
public IndicesOptions indicesOptions() { | ||
return INDICES_OPTIONS; | ||
} |
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.
why do we need to override this method?
} | ||
throw new IllegalArgumentException("Alias [" + expression + | ||
"] has multiple write indices " + Arrays.toString(indexNames) | ||
+ " associated with it with [is_write_index = true]"); |
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.
is this something that can happen in reality? I was under the impression that we would try and make sure that an alias that points to multiple indices has a single write index. Feels a bit weird to have to check for this every time we resolve an index expression.
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.
We do not upgrade existing alias configurations, or auto-set is_write_index
for alias update api calls. So indices may remain with no write indices
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.
but here we are validating that an alias does not have multiple write indices rather than no write indices?
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.
oh, I see it now. sorry. did not expand for context. You're right. This is a leftover exception from a previous way of doing things. I will clean this up. thanks!
@@ -194,14 +194,27 @@ public IndexNameExpressionResolver(Settings settings) { | |||
} | |||
|
|||
Collection<IndexMetaData> resolvedIndices = aliasOrIndex.getIndices(); | |||
if (resolvedIndices.size() > 1 && !options.allowAliasesToMultipleIndices()) { |
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.
given that this conditional is replaced, does that mean that allowAliasesToMultipleIndices is now always used together with the new requireAliasesToWriteIndex ? Or are there still cases where we don't allow aliases to multiple indices, yet we don't require aliases to write indices?
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.
if we do not allow multiple indices and do not require a write index... the normal resolution that occurs today where alias will resolve to the only index associated with it is still OK, even if it is not the write index.
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 am not following where that happens though, as the conditional for allowAliasesToMultipleIndices
alone has become the conditional for allowAliasesToMultipleIndices && requireAliasesToWriteIndex
. Do we have the old behaviour somewhere else which addresses the case where allowAliasesToMultipleIndices && requireAliasesToWriteIndex ==false
?
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.
allowAliasesToMultipleIndices && requireAliasesToWriteIndex ==false
occurs during searching, no?
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.
argh, the exclamation mark bites again. I thought that the new if was the same as before plus a new condition.
What still confuses me here though is that the old conditional if (resolvedIndices.size() > 1 && !options.allowAliasesToMultipleIndices())
is gone as well as the IllegalArgumentException
thrown in such case.
What happens now when allowAliasesToMultipleIndices is false? Does it never happen that allowAliasesToMultipleIndices is false? I kind of imply from this change that there are no API left with allowAliasesToMultipleIndices set to false? Do I read this correctly?
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.
get,mget do not allow aliases to multiple indices
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.
get,mget do not allow aliases to multiple indices
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.
cool, thanks. where do we throw exception for them in case they are called and an alias that points to multiple indices is provided? Isn't that the codepath that's being replaced here?
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.
@javanna I was confused about this as well, but I believe this is caught somewhere else, I forget exactly where. I am positive it is, because there is a rest test for mget (using same indicesOptions as get) that verifies this: https://github.com/elastic/elasticsearch/blob/2a65bed243a0387bdf1175c0ca4b73ed863d9792/rest-api-spec/src/main/resources/rest-api-spec/test/mget/14_alias_to_multiple_indices.yml
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.
ok I will dig and find out, too curious now :)
I didn't do a full review, but I left some questions on the core changes made. |
thanks for providing your feedback @javanna. I'll address them soon! |
Hi @bleskes, I believe this PR is ready for a real review now. thank you for your patience! |
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.
@talevy the gist looks good. I didn't do a full review but I think I left enough comments for an iteration. Also, I think you need to pay attention to BWC and cluster state recovery. I can help out with it on monday.
[[aliases-write-index]] | ||
==== Write Index | ||
|
||
It is possible to associate the index associated to an alias as the write index for this |
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.
double associate* reads clunky.
} | ||
|
||
/** | ||
* Associates a write-only alias boolean flag to the alias |
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.
nit: I think you want to stay "set" as you can technically set the flag to false.
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.
you're right
@@ -189,6 +208,9 @@ public void writeTo(StreamOutput out) throws IOException { | |||
out.writeOptionalString(filter); | |||
out.writeOptionalString(indexRouting); | |||
out.writeOptionalString(searchRouting); | |||
if (out.getVersion().onOrAfter(Version.V_6_4_0)) { |
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.
hm... if this works we have some rest test coverage issue as this PR is not yet backported. Can you start with 7.0.0 and then update using the bwc disable/enable song and dance? (I can explain more if needed).
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 get it, yup
@@ -215,6 +217,7 @@ public static AliasActions removeIndex() { | |||
private String routing; | |||
private String indexRouting; | |||
private String searchRouting; | |||
private boolean writeIndex; |
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.
Should this be Boolean
? How does one indicate "default"?
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.
since it defaults to false
when not set, I figured it was OK. I will change it
// CONSOLE | ||
// TEST[s/^/PUT test\nPUT test2\n/] | ||
|
||
In this example, we associate the alias `alias1` to both `test` and `test2`, where |
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.
can you add an example (or properly note in this one) of how to shift the write alias from one index to another? I think the use of "add" on the index that had the write alias to unset is is confusing and we should document it well.
} | ||
} | ||
|
||
public void validateAliasWriteOnly(String checkAlias, String checkIndex, boolean writeIndex, |
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.
this is not called anywhere?
* @param metaData the Cluster metadata | ||
* @throws IllegalArgumentException if the alias cannot be write_only | ||
*/ | ||
public void validateAliasWriteOnly(String aliasName, boolean isWriteIndex, MetaData metaData) { |
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.
this not called anywhere in unit tests. Can we add some?
return writeIndex && getExistingWriteIndex(aliasName, indices) == null; | ||
} | ||
|
||
public static boolean validAliasWriteOnly(String aliasName, boolean writeIndex, ImmutableOpenMap<String, IndexMetaData> indices) { |
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.
can we get another naming convention? we use validate for exceptions and valid is just too close. Also, unit tests please.
int i = 0; | ||
for (IndexMetaData indexMetaData : resolvedIndices) { | ||
indexNames[i++] = indexMetaData.getIndex().getName(); | ||
if (resolvedIndices.size() > 1 && options.allowAliasesToMultipleIndices() && options.requireAliasesToWriteIndex()) { |
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.
Indeed, I think we should maintain behavior and throw an exception if resolvedIndices.size() > 1 and options. requireAliasesToWriteIndex() == false and options.allowAliasesToMultipleIndices() == false too. Also can you assert in the indexOptions class that options.allowAliasesToMultipleIndices() and requireAliasesToWriteIndex() are never used together? it's confusing.
List<IndexMetaData> writeIndices = alias.getWriteIndices(); | ||
if (writeIndices.size() == 1) { | ||
resolvedIndices = writeIndices; | ||
} else if (writeIndices.size() > 1) { |
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.
When can this happen (question from before)? index write is always unique?
@@ -127,7 +127,7 @@ ClusterState innerExecute(ClusterState currentState, Iterable<AliasAction> actio | |||
if (index == null) { | |||
throw new IndexNotFoundException(action.getIndex()); | |||
} | |||
NewAliasValidator newAliasValidator = (alias, indexRouting, filter) -> { | |||
NewAliasValidator newAliasValidator = (alias, indexRouting, filter, writeIndex) -> { |
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.
this parameter isn't used anywhere. I think it should validate writeIndex semantics are sound?
@talevy I spent some time thinking about how we can break this PR into smaller ones. How about we do one PR to introduce the write alias flag and a follow up to actually use it? |
that sounds reasonable. I’ll open a new one with just the flag added when I come back. thanks, good idea! |
The Rest Put-Alias Action does separate parsing of the alias body to construct the IndicesAliasesRequest. This extra parsing was missed in elastic#30703.
* add support for is_write_index in put-alias body parsing The Rest Put-Alias Action does separate parsing of the alias body to construct the IndicesAliasesRequest. This extra parsing was missed in #30703. * test flag was not just ignored by the parser * disable backcompat tests
* add support for is_write_index in put-alias body parsing The Rest Put-Alias Action does separate parsing of the alias body to construct the IndicesAliasesRequest. This extra parsing was missed in #30703. * test flag was not just ignored by the parser
This commit introduces the concept of an
is_write_index
flag foraliases. Before, it was invalid to write to indices which point to multiple
indices. Now, users can configure one index to have an alias with
is_write_index: true
and others that are false. When one attempts to index to this alias, it will resolve to
the one index that is the write index.
in response to #30061.
This PR introduces a concept in aliases called
is_write_index
. Whentrue
on a specificalias definition associated with an
index
, index requests using the associated alias name willresolve to this
index
for indexing.Write now, if one attempts to index a document using an alias that points to multiple indices, there is no way to resolve which one, so an exception is thrown. With this flag, you can have an alias that points to a specific index in a set of indices as the one to write to.
It is possible to set an index as an alias' write-index during index creation:
Now, index requests like
PUT bar/_doc/_id
will route to indexfoo1
. Another index,foo2
, isfree to also have an alias named "bar" pointing to it. This alias needs to be configured with
is_write_index
set tofalse
.is_write_index
is defaulted tofalse
if not specified, or it can be explicitly set tofalse
.Now, searches against
bar
will point to both indices, but index requests will be routed tofoo1
.These settings can be updated in the
_aliases
API like so:How does this relate to Rollover? Rollover depended on rollover-aliases that only point to
one index since multiple would break indexing. Now that the same alias can point to multiple indices, the only requirement we have is that there is only one write index.
Here is an example scenario that didn't work before, but will work after Rollover is updated
to leverage the
is_write_index
use-case.