Datastream unavailable exception metadata by albertzaharovits · Pull Request #91461 · elastic/elasticsearch

albertzaharovits · 2022-11-09T13:24:27Z

The goal of this PR is to reuse the "resource unavailable" exception conditions,
between when wildcards are expanded and when they're not.
It's a (big) step in that direction, rather than a complete work.
I have stopped short of a complete implementation because
I became aware of a required behavior change that I think needs
analyzed and discussed in advance, so that I can confidently proceed.

I'll explain commit-by-commit:

19a4128 When wildcards are expanded, explicitly naming datastreams when the context disallows datastreams, and also when ignore_unavailable is false, will generate a "not found" exception, see:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

Line 1274 in 6e60e6a

throw indexNotFoundException(expression);

However, in the same context that doesn't allow datastreams, but without expanding wildcards, datastreams are always ignored, even if ignore_unavailable is false. This commit fixes that.
938d2c0 Is the behavior change that I think needs to be discussed. Some "index not found" exceptions have a metadata field es.excluded_ds: true. The field is only used in a single place, and only for update requests (which don't support datastreams, and only target a single index):

elasticsearch/server/src/main/java/org/elasticsearch/action/support/single/instance/TransportInstanceSingleOperationAction.java

Line 146 in d5b4584

if (request.includeDataStreams() == false && e.getMetadataKeys().contains(EXCLUDED_DATA_STREAMS_KEY)) {

. It is however serialized, so it is visible to callers.
- Curently, the metadata field is attached when an expression evalutes to the empty concrete indices list, and at least a datastreams was ignored in the process (either because ignore_unavailable is true or because, as described above (and fixed), datastreams are always ignored when wildcards are not expanded).
- The proposed PR will attach the metadata field whenever a datastream is explicitly named in a context that disallows datastreams and ignore_unavailable is false.
- Both approaches are equivalent with respect to the internal usage of the field (used only in the context of the update request, that disallows datastreams, refers to a single shard, and ignore_unavailable is false), but the metadata field is visible to callers.
- I want to introduce this change is because I want to separate the layers that 1) resolve an expression to a list of index, alias, and datastream names (which will be reused in Security), and 2) expand the elements of that list to concrete indices (which will remain in Core). I cannot achieve the separation if the layer that expands to concrete indices (2) has to know about ignored datastreams, which are handled in the other layer.
d1f11d4 Reuses the method that generates "not found" exceptions. In this context I realised that for a single "unavailable" name, sometimes the exception was constructed with infe.setResources("index_or_alias", expression); and sometimes with infe.setResources("index_expression", expression);; I opted to always use index_or_alias.
6b2dc8e This shows the direction I'm going to, I will iterate on it to encapsulate the handling of unavailable resources inside the resolveExpressions method.

Is the slight behavior change (circumstances when the "not found" exceptions contain a metadata field, without impacting any other behaviors, but that can be observed by callers in the serialized exception, as detailed above) a concern for anybody?

albertzaharovits · 2022-11-09T13:52:29Z

@elasticmachine run elasticsearch-ci/part-1

albertzaharovits · 2022-11-09T15:56:15Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

+            if (options.ignoreUnavailable() == false) {
+                ensureAliasOrIndexExists(context, expression);
+            }
            IndexAbstraction indexAbstraction = indicesLookup.get(expression);
            if (indexAbstraction == null) {
-                if (options.ignoreUnavailable() == false) {
-                    assert options.expandWildcardExpressions() == false;
-                    throw notFoundException(expression);
-                } else {
-                    continue;
-                }
+                continue;
            } else if (indexAbstraction.getType() == Type.ALIAS && context.getOptions().ignoreAliases()) {
-                if (options.ignoreUnavailable() == false) {
-                    assert options.expandWildcardExpressions() == false;
-                    throw aliasesNotSupportedException(expression);
-                } else {
-                    continue;
-                }
+                continue;
            } else if (indexAbstraction.isDataStreamRelated() && context.includeDataStreams() == false) {
-                excludedDataStreams = true;
                continue;
            }



All of this is going to be moved, in a follow-up PR, inside the resolveExpression method on line 339.

elasticsearchmachine · 2022-11-09T16:03:34Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

grcevski · 2022-11-09T19:04:20Z

This sounds good to me. I can't find anyone referring to es.excluded_ds and it's not documented, I think we can safely say it was only for the internal use of our own transport action. Do you know if it's mentioned in any client specs? Again, I can't find it, but I just want to make sure I didn't miss a place.

albertzaharovits · 2022-11-10T08:22:19Z

Do you know if it's mentioned in any client specs? Again, I can't find it, but I just want to make sure I didn't miss a place.

No, I can't find references to it (it eventually gets serialized in the exception body of the response as excluded_ds). The rest specs that I'm aware of (rest-api-spec project) are not concerned with responses at all. In addition I know we shouldn't change the "shape" of responses, and I also believe we're even laxer with error response bodies (though I admit I'm not sure of specific guidelines). I have asked the lang clients team for advice.

albertzaharovits · 2022-11-10T12:48:50Z

I have asked the lang clients team for advice.

@swallez confirmed on another channel that client code doesn't look at exception metadata fields.

grcevski

LGTM!

jakelandis

LGTM

Can you add the bug label to the PR ? I think at least the first commit fixes a bug and the second one is arguable a bug too.

elasticsearchmachine · 2022-11-14T11:46:17Z

Hi @albertzaharovits, I've created a changelog YAML for you.

albertzaharovits · 2022-11-14T11:48:45Z

@elasticsearchmachine update branch

* main: (163 commits) [DOCS] Edits frequent items aggregation (elastic#91564) Handle providers of optional services in ubermodule classloader (elastic#91217) Add `exportDockerImages` lifecycle task for exporting docker tarballs (elastic#91571) Fix CSV dependency report output file location in DRA CI job Fix variable placeholder for Strings.format calls (elastic#91531) Fix output dir creation in ConcatFileTask (elastic#91568) Fix declaration of dependencies in DRA snapshots CI job (elastic#91569) Upgrade Gradle Enterprise plugin to 3.11.4 (elastic#91435) Ingest DateProcessor (small) speedup, optimize collections code in DateFormatter.forPattern (elastic#91521) Fix inter project handling of generateDependenciesReport (elastic#91555) [Synthetics] Add synthetics-* read to fleet-server (elastic#91391) [ML] Copy more settings when creating DF analytics destination index (elastic#91546) Reduce CartesianCentroidIT flakiness (elastic#91553) Propagate last node to reinitialized routing tables (elastic#91549) Forecast write load during rollovers (elastic#91425) [DOCS] Warn about potential overhead of named queries (elastic#91512) Datastream unavailable exception metadata (elastic#91461) Generate docker images and dependency report in DRA ci job (elastic#91545) Support cartesian_bounds aggregation on point and shape (elastic#91298) Add support for EQL samples queries (elastic#91312) ... # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java

albertzaharovits added 2 commits November 9, 2022 15:04

handle ignore unavailable for datastream related

19a4128

Move EXCLUDED_DATA_STREAMS_KEY

938d2c0

elasticsearchmachine added the v8.6.0 label Nov 9, 2022

albertzaharovits added 2 commits November 9, 2022 15:57

Reuse method notFoundException

d1f11d4

Reuse ensureAliasOrIndexExists

6b2dc8e

albertzaharovits self-assigned this Nov 9, 2022

albertzaharovits commented Nov 9, 2022

View reviewed changes

albertzaharovits added >non-issue :Core/Infra/Core Core issues without another label labels Nov 9, 2022

albertzaharovits requested review from grcevski and jakelandis and removed request for jakelandis November 9, 2022 16:02

albertzaharovits changed the title ~~Datastream not found error~~ Datastream unavailable exception metadata Nov 9, 2022

albertzaharovits marked this pull request as ready for review November 9, 2022 16:03

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Nov 9, 2022

grcevski approved these changes Nov 10, 2022

View reviewed changes

jakelandis approved these changes Nov 10, 2022

View reviewed changes

albertzaharovits added >bug and removed >non-issue labels Nov 14, 2022

Update docs/changelog/91461.yaml

be5d5cb

Merge branch 'main' into datastream-not-found-error

7b9ce3c

albertzaharovits merged commit 6fc31d3 into elastic:main Nov 14, 2022

albertzaharovits deleted the datastream-not-found-error branch November 14, 2022 12:40

albertzaharovits mentioned this pull request Nov 20, 2022

Handle explicit names in separate step #91408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datastream unavailable exception metadata#91461

Datastream unavailable exception metadata#91461
albertzaharovits merged 6 commits intoelastic:mainfrom
albertzaharovits:datastream-not-found-error

albertzaharovits commented Nov 9, 2022 •

edited

Loading

Uh oh!

albertzaharovits commented Nov 9, 2022

Uh oh!

albertzaharovits Nov 9, 2022

Uh oh!

elasticsearchmachine commented Nov 9, 2022

Uh oh!

grcevski commented Nov 9, 2022

Uh oh!

albertzaharovits commented Nov 10, 2022 •

edited

Loading

Uh oh!

albertzaharovits commented Nov 10, 2022

Uh oh!

grcevski left a comment

Uh oh!

jakelandis left a comment

Uh oh!

elasticsearchmachine commented Nov 14, 2022

Uh oh!

albertzaharovits commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

albertzaharovits commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertzaharovits commented Nov 9, 2022

Uh oh!

albertzaharovits Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Nov 9, 2022

Uh oh!

grcevski commented Nov 9, 2022

Uh oh!

albertzaharovits commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertzaharovits commented Nov 10, 2022

Uh oh!

grcevski left a comment

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Nov 14, 2022

Uh oh!

albertzaharovits commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

albertzaharovits commented Nov 9, 2022 •

edited

Loading

albertzaharovits commented Nov 10, 2022 •

edited

Loading