-
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
add support for write index resolution when creating/updating documents #31520
Conversation
Pinging @elastic/es-core-infra |
f3e678c
to
d2da8ec
Compare
@talevy thx. I had a look and some thinking time and I would like to take a somewhat (but not complete) different approach.
How does that sound? |
@bleskes thanks for taking a look
|
This commit introduces a new option to IndicesOptions that requires a write index to exist and then uses this option for index/update requests. This commit also fixes a subtle issue with how write-indices are resolved in Aliases. Before, all aliases pointing to one-and-only-one index had a write index even if is_write_index=false. This should not be the case.
Another assumption I had was that |
d2da8ec
to
d497336
Compare
d497336
to
df8fabd
Compare
@@ -46,13 +46,15 @@ | |||
*/ | |||
public class ReindexSourceTargetValidationTests extends ESTestCase { |
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 class needs more work, but I am not sure whether we want reindex target-aliases to resolve to
the write index. It feels right, but it also sounds scary. 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.
oh nevermind, it must check details since even a single index with is_write_index=false
should not be chosen by reindex api, and that would be awkward
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'm not sure I follow. Reindex should be orthogonal to the write alias and should just work ™️ ? what am I missing?
@bleskes I've updated to remove the flag from IndicesOptions. I still have some follow-up work to do with handling ReindexRequests, but I think it is in a place that can be reviewed. Specifically regarding these two points
|
I haven't looked into this in depth, but I see changes being made in how some actions resolve indices which makes me think that these changes should also be taken into account in the security plugin indices resolution code (see https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java). Have these changes been tested with security installed? The reason why As a long-term plan it would be beneficial to unify indices resolution as described in #29915, so that this type of changes are automatically reflected in the security indices resolution logic (or hopefully security gets to have less and less of its own logic). I never got to work on that unfortunately, not even sure how hard that's going to be. I would love if we could also work on #9438 as a follow-up, I am happy that we did not end up adding yet another option to |
thanks for providing all these details @javanna! I wasn't aware of any of that. I'll double check how Security interacts with this resolution and be sure it is tested |
Just chatted to @bleskes and we agreed that this deserves a test, but we think that this change should not impact security as it only has to do with how aliases get expanded, while our security plugin leaves aliases untouched. |
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 think this direction is good. I left some initial comments.
@@ -46,13 +46,15 @@ | |||
*/ | |||
public class ReindexSourceTargetValidationTests extends ESTestCase { |
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'm not sure I follow. Reindex should be orthogonal to the write alias and should just work ™️ ? what am I missing?
@@ -109,7 +112,7 @@ private void succeeds(RemoteInfo remoteInfo, String target, String... sources) { | |||
INDEX_NAME_EXPRESSION_RESOLVER, AUTO_CREATE_INDEX, STATE); | |||
} | |||
|
|||
private static IndexMetaData index(String name, String... aliases) { | |||
private static IndexMetaData index(String name, boolean isWriteIndex, String... aliases) { |
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 param isn't used?
@@ -295,7 +295,7 @@ protected void doRun() throws Exception { | |||
TransportUpdateAction.resolveAndValidateRouting(metaData, concreteIndex.getName(), (UpdateRequest) docWriteRequest); | |||
break; | |||
case DELETE: | |||
docWriteRequest.routing(metaData.resolveIndexRouting(docWriteRequest.routing(), docWriteRequest.index())); | |||
docWriteRequest.routing(metaData.resolveIndexRouting(docWriteRequest.routing(), docWriteRequest.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.
double checking - do we have a test for multi routing values in alias and make sure we use the right value (I hate this API but we have to test it works)?
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.
With some more thinking, I don't think we should make this feature (routing value in an alias) work with the write index flag. The problem is that if we allow to write to multi index aliases with this feature turned on, there is no way for get to work through those aliases without us changing the get behavior of resolving multi-index aliased base on the index write flag which is unexpected. Bottom line, can we see what it takes to make sure resolveIndexRouting explodes as soon as you touch more than one index via an alias?
* @throws IllegalArgumentException if one of the aliases resolve to multiple indices and the provided | ||
* indices options in the context don't allow such a case. | ||
*/ | ||
public Index[] concreteIndices(ClusterState state, IndicesOptions options, boolean resolveToWriteIndex, String... indexExpressions) { |
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 does this method need to be public ? can we remove it and inline the implementation with it's single caller? it's just two lines.
@@ -194,29 +213,43 @@ public IndexNameExpressionResolver(Settings settings) { | |||
} | |||
|
|||
Collection<IndexMetaData> resolvedIndices = aliasOrIndex.getIndices(); |
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 make if clause have this structure and inline this variable in the second else (see marker):
if (aliasOrIndex.isAlias() && context.isResolveToWriteIndex()) {
.. write operation path
} else {
... read path ..
start the old code here and put the resolvedIndices variable definition 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.
ping about inlining this variable.
String indexExpression = request.indices() != null && request.indices().length > 0 ? request.indices()[0] : null; | ||
Index[] indices = concreteIndices(state, request.indicesOptions(), true, indexExpression); | ||
if (indices.length != 1) { | ||
throw new IllegalArgumentException("unable to return a single index as the index and options provided got resolved 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.
I think we need to adapt this message for more things that can go wrong (single index with no write flag etc.)
@@ -477,7 +477,7 @@ private static String mergePaths(String path, String field) { | |||
*/ | |||
// TODO: This can be moved to IndexNameExpressionResolver too, but this means that we will support wildcards and other expressions | |||
// in the index,bulk,update and delete apis. | |||
public String resolveIndexRouting(@Nullable String routing, String aliasOrIndex) { | |||
public String resolveIndexRouting(@Nullable String routing, String aliasOrIndex, boolean isWriteOperation) { |
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.
see my comment earlier
re Reindex being orthogonal, that may be the case. I will double check I am understanding the logic. This code seemed relevant for resolving the concrete target index: https://github.com/elastic/elasticsearch/blob/master/modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportReindexAction.java#L185 |
Hey @bleskes,
thanks! |
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 think this approach looks great. I left some minor comments
AliasOrIndex.Alias alias = (AliasOrIndex.Alias) aliasOrIndex; | ||
IndexMetaData writeIndex = alias.getWriteIndex(); | ||
if (writeIndex == null) { | ||
if (alias.getIndices().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.
I don't think you can make this statement - it may be that all of them have their write index flag set to false. I think we can have a generic statement like "no write index is defined for alias X. The write index may be explicitly disabled using is_write_index=false
or the alias points to multiple indices without one being designated as a 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 will make more generic
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.
it may be that all of them have their write index flag set to false
saying it points to multiple indices with none set as a write-index [is_write_index=true] does not conflict with that statement?
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.
isn't this covered by "the alias points to multiple indices without one being designated as a write index"?
} | ||
Context context = new Context(state, request.indicesOptions(), false, true); | ||
Index[] indices = concreteIndices(context, request.indices()[0]); | ||
if (indices.length != 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.
💯
} | ||
AliasMetaData aliasMd = writeIndex.getAliases().get(alias.getAliasName()); | ||
if (aliasMd.indexRouting() != null) { | ||
if (aliasMd.indexRouting().indexOf(',') != -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.
I wonder if we should validate that when people set is_write_index to true as well
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 think that to do this validation justice, it will have to be looked at as something that can be invalid for both read and write. Since this check is similar in both. For this reason, I would feel better if that stricter validation be looked at outside this PR
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 agree it's not straight forward. Let's have another PR (I tend to say that multiple routing values changes the default for is_write_index and disallows setting it)
@@ -132,4 +160,55 @@ public void testDeleteNonExistingDocExternalGteVersionCreatesIndex() throws Exce | |||
throw new AssertionError(exception); | |||
})); | |||
} | |||
|
|||
public void testRoutingResolvesToWriteIndexAliasMetaData() throws Exception { |
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 wonder if we need a test at this level. We already unit test MetaData and we have an IT test to check it works end to end.
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.
actually, I don't think we have an IT test, but we should
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 couldn't find an IT test that verifies this, and I thought that this level of unit test was sufficient. Will add IT test (will also double check that one doesn't exist)
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 think this can be removed now that we have an IT test? this layer doesn't add anything to the write index resolving as it is all in the resolver? it's just glue and is covered well by the IT?
Arrays.sort(strings); | ||
assertArrayEquals(new String[] {"test-alias"}, strings); | ||
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, | ||
() -> indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.strictSingleIndexNoExpandForbidClosed(), |
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 doesn't specify anything concerning a write request but you never set the write flag on the aliases. I think we can also randomize setting it?
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. this test was not added for checking write index resolution. This test was added because there was no unit test that hit this branch of the code beforehand. I will add a write-index to the mix for completion
.numberOfShards(1) | ||
.numberOfReplicas(0) | ||
.putAlias(AliasMetaData.builder("alias0").build()) | ||
.putAlias(AliasMetaData.builder("alias1").routing("1").build()) |
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 don't have an explicit is_write_index set to true in this test. Set it here randomly?
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'll take a look. I didn't add it here since I didn't feel like it was this test's duty to worry about such things since getWriteIndex
should be doing that test
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.
didn't hurt to add. added
.numberOfReplicas(0) | ||
.putAlias(AliasMetaData.builder("alias0").build()) | ||
.putAlias(AliasMetaData.builder("alias1").routing("1").build()) | ||
.putAlias(AliasMetaData.builder("alias2").routing("1,2").build()) |
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 don't believe we check rejection of multi value routings with is_write_index set to true. Can we add a test that it fails (preferably but rejecting it when building the metadata objects?)
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 will add a new alias4
with is_write_index=true
and have it fail.
(preferably but rejecting it when building the metadata objects?)
I think this touches the previous comment about failing invalid routing values early. Although I agree, I do not feel comfortable introducing this change in this PR
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.
++
- cleaned up exception message for when no write aliases are set and being resolved - added integration test for Get Action against alias pointing to multiple indices - added integration test for bulk with routing and resolution to write index alias 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.
Left some very minor nits. LGTM otherwise
assertThat(getBuilder.get().getSource().get("foo"), equalTo("bar")); | ||
assertThat(client().prepareGet("index3", "type", "id").setRouting("1").get().getSource().get("foo"), equalTo("baz")); | ||
|
||
client().prepareBulk().add(client().prepareUpdate("alias1", "type", "id").setDoc("foo", "updated")).get(); |
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 should check errors 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.
will add. didn't think this was necessary since there are get-action calls verifying success of this operation
client().prepareBulk().add(client().prepareUpdate("alias1", "type", "id").setDoc("foo", "updated")).get(); | ||
assertThat(getBuilder.get().getSource().get("foo"), equalTo("bar")); | ||
assertThat(client().prepareGet("index3", "type", "id").setRouting("1").get().getSource().get("foo"), equalTo("updated")); | ||
client().prepareBulk().add(client().prepareDelete("alias1", "type", "id")).get(); |
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'm not sure what we're testing 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.
TransportBulkAction resolves routing for delete requests just as it does for index and update. This delete request is here to verify this
indexRequest.routing("0"); | ||
} | ||
indexRequest.source(Collections.singletonMap("foo", "bar")); | ||
IndexRequest indexRequestWithAlias = new IndexRequest("alias1", "type", "id"); |
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 use different ids for the different indices? I find this super confusing to reason about. Maybe also add the routing value you expect to be used to the id.
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 think it is also OK to remove the first index request entirely. I just wanted to it to demonstrate that the two calls do not interact with the same document, even though they have the same index/type/id.
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.
On second thought, I remember the purpose of this other index request. It is to verify the other branches of the routing resolution. The reason I think it is OK to remove is that this IT test is meant to test the connection to this method, not all its branches. The Unit tests do that.
@@ -132,4 +160,55 @@ public void testDeleteNonExistingDocExternalGteVersionCreatesIndex() throws Exce | |||
throw new AssertionError(exception); | |||
})); | |||
} | |||
|
|||
public void testRoutingResolvesToWriteIndexAliasMetaData() throws Exception { |
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 think this can be removed now that we have an IT test? this layer doesn't add anything to the write index resolving as it is all in the resolver? it's just glue and is covered well by the IT?
.numberOfReplicas(0) | ||
.putAlias(AliasMetaData.builder("alias0").build()) | ||
.putAlias(AliasMetaData.builder("alias1").routing("1").build()) | ||
.putAlias(AliasMetaData.builder("alias2").routing("1,2").build()) |
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.
++
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.
LGTM. Thanks @talevy for all the hard work.
client().admin().indices().prepareCreate("index3") | ||
.addAlias(new Alias("alias1").indexRouting("1").writeIndex(true)).setSettings(twoShardsSettings).get(); | ||
|
||
IndexRequest indexRequestWithAlias = new IndexRequest("alias1", "type", "id"); |
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.
randomly set the routing to "1"?
thanks for the thorough review iterations @bleskes |
…ts (elastic#31520) Now write operations like Index, Delete, Update rely on the write-index associated with an alias to operate against. This means writes will be accepted even when an alias points to multiple indices, so long as one is the write index. Routing values will be used from the AliasMetaData for the alias in the write-index. All read operations are left untouched.
…ts (#31520) Now write operations like Index, Delete, Update rely on the write-index associated with an alias to operate against. This means writes will be accepted even when an alias points to multiple indices, so long as one is the write index. Routing values will be used from the AliasMetaData for the alias in the write-index. All read operations are left untouched.
* master: Painless: Simplify Naming in Lookup Package (#32177) Handle missing values in painless (#32207) add support for write index resolution when creating/updating documents (#31520) ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864) Remove indication of future multi-homing support (#32187) Rest test - allow for snapshots to take 0 milliseconds Make x-pack-core generate a pom file Rest HL client: Add put watch action (#32026) Build: Remove pom generation for plugin zip files (#32180) Fix comments causing errors with Java 11 Fix rollup on date fields that don't support epoch_millis (#31890) Detect and prevent configuration that triggers a Gradle bug (#31912) [test] port linux package packaging tests (#31943) Revert "Introduce a Hashing Processor (#31087)" (#32178) Remove empty @return from JavaDoc Adjust SSLDriver behavior for JDK11 changes (#32145) [test] use randomized runner in packaging tests (#32109) Add support for field aliases. (#32172) Painless: Fix caching bug and clean up addPainlessClass. (#32142) Call setReferences() on custom referring tokenfilters in _analyze (#32157) Fix BwC Tests looking for UUID Pre 6.4 (#32158) Improve docs for search preferences (#32159) use before instead of onOrBefore Add more contexts to painless execute api (#30511) Add EC2 credential test for repository-s3 (#31918) A replica can be promoted and started in one cluster state update (#32042) Fix Java 11 javadoc compile problem Fix CP for namingConventions when gradle home has spaces (#31914) Fix `range` queries on `_type` field for singe type indices (#31756) [DOCS] Update TLS on Docker for 6.3 (#32114) ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are Remove versionType from translog (#31945) Switch distribution to new style Requests (#30595) Build: Skip jar tests if jar disabled Painless: Add PainlessClassBuilder (#32141) Build: Make additional test deps of check (#32015) Disable C2 from using AVX-512 on JDK 10 (#32138) Build: Move shadow customizations into common code (#32014) Painless: Fix Bug with Duplicate PainlessClasses (#32110) Remove empty @param from Javadoc Re-disable packaging tests on suse boxes Docs: Fix missing example script quote (#32010) [ML] Wait for aliases in multi-node tests (#32086) [ML] Move analyzer dependencies out of categorization config (#32123) Ensure to release translog snapshot in primary-replica resync (#32045) Handle TokenizerFactory TODOs (#32063) Relax TermVectors API to work with textual fields other than TextFieldType (#31915) Updates the build to gradle 4.9 (#32087) Mute :qa:mixed-cluster indices.stats/10_index/Index - all’ Check that client methods match API defined in the REST spec (#31825) Enable testing in FIPS140 JVM (#31666) Fix put mappings java API documentation (#31955) Add exclusion option to `keep_types` token filter (#32012) [Test] Modify assert statement for ssl handshake (#32072)
* es/6.x: (24 commits) Fix broken backport Switch full-cluster-restart to new style Requests (#32140) Fix multi level nested sort (#32204) MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248) [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236) Switch rolling restart to new style Requests (#32147) Enhance Parent circuit breaker error message (#32056) [ML] Use default request durability for .ml-state index (#32233) Enable testing in FIPS140 JVM (#31666) (#32231) Remove indices stats timeout from monitoring docs TESTS: Check for Netty resource leaks (#31861) (#32225) Rename ranking evaluation response section (#32166) Dependencies: Upgrade to joda time 2.10 (#32160) Backport SSL context names (#30953) to 6.x (#32223) Require Gradle 4.9 as minimum version (#32200) Detect old trial licenses and mimic behaviour (#32209) Painless: Simplify Naming in Lookup Package (#32177) add support for write index resolution when creating/updating documents (#31520) A replica can be promoted and started in one cluster state update (#32042) Rest test - allow for snapshots to take 0 milliseconds ...
Using the document update API on aliases with a write index does not work. Follow-up to #31520
Using the document update API on aliases with a write index does not work. Follow-up to #31520
Using the document update API on aliases with a write index does not work. Follow-up to #31520
This commit introduces a new option to IndicesOptions that requires
a write index to exist and then uses this option for index/update requests.
Get requests ignore the write index.