Route documents to the correct shards in tsdb #77731

nik9000 · 2021-09-14T20:44:34Z

This causes elasticsearch to land documents from the same time series on
the same shard. It does so by adding a new index setting routing_path
which must be set when an index is in mode: time_series and may not be
set outside of that mode. That setting contains a list of patterns to
extract from the _source document that are hashed into the routing
value.

Note: This doesn't guarantee that the routing_path only matches dimensions.
For now it is possible to configure the routing_path so it matches non-dimension
fields. This would the same time series to end up in multiple shards. We don't want
that and plan to add that constraint in a follow up change.

nik9000 · 2021-09-14T20:47:23Z

In addition to routing documents to the correct shard this adds tsdb's restriction turning off delete and update and indexing with a specific routing or id.

This causes elasticsearch to land documents from the same time series on the same shard. It does so by adding a new index setting `routing_path` which must be set when an index is in `mode: time_series` and may not be set outside of that mode. That setting contains a list of patterns to extract from the `_source` document that are hashed into the routing value.

imotov

Looks good to me from TSDB perspective. It doesn't have everything we need, but it is a good start.

...multinode/src/test/resources/rest-api-spec/test/smoke_test_multinode/20_tsdb_consistency.yml

build-tools-internal/src/main/groovy/elasticsearch.formatting.gradle

henningandersen

Thanks Nik, I left a few comments. I did not find a restriction on GET in the design doc, hence the question on that, we may need to sync up on that.

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

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

server/src/main/java/org/elasticsearch/cluster/routing/IndexRouting.java

We do want to reject these documents but let's sae that for a follow up change.

elasticmachine · 2021-09-16T18:40:28Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

henningandersen

Thanks Nik, I left a few comments. The main concern is about forking from the bulk action when the request contains tsdb items.

henningandersen · 2021-09-23T09:30:09Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

@@ -462,28 +468,32 @@ protected void doRun() {
                            Version indexCreated = indexMetadata.getCreationVersion();
                            indexRequest.resolveRouting(metadata);
                            indexRequest.process(indexCreated, mappingMd, concreteIndex.getName());
+                            shardId = indexRouting.indexShard(


nit: I wonder if we can move this to the DocWriteRequest with sub-class implementations instead? Would simplify the code here a bit. Would need to accept the IndexRouting object as a parameter to enable the caching here.

Optional of course.

I thought about doing that when I was out on a walk a while back. I didn't like it at the time. But I'll give it a shot and see how it looks.

I don't think I'm going to try this in this PR. But I will keep it in mind. I like it and I don't. But I'll remember it for sure.

I am ok to keep it as is for now, but happy to hear your more elaborate thoughts on the pros and cons of this?

Over the last month I've had a bit of a change of heart here. I'd love to give it a go, but I'm going to hold it for a follow up. That way if we look at it and say "oh no, that's icky" we can just not merge it.

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

nik9000 · 2021-09-23T21:48:20Z

@henningandersen I pushed a change that adds the forking. I'd love it if you could see if it looks like it is along the right lines. I haven't had a chance to write new tests for it, but the old tests seem happy enough.

nik9000 · 2021-09-23T21:49:59Z

old tests seem happy enough.

Not quite! DataStreams tests failed. I'll have a look on Monday.

henningandersen

LGTM, though I think it makes sense to get a review from a tsdb team member too.

henningandersen · 2021-10-14T08:51:09Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

+                    threadPool.executor(executorName).submit(new ActionRunnable<>(listener) {
+                        @Override
+                        protected void doRun() throws Exception {
+                            threadPool.executor(executorName).execute(new ActionRunnable<>(listener) {


I think we need just one level of dispatch here?

👍 Oh boy. That's a strange way to write that. I'm wondering how I got there....

henningandersen · 2021-10-14T09:02:57Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

@@ -394,37 +406,46 @@ private long buildTookInMillis(long startTimeNanos) {
     * retries on retryable cluster blocks, resolves item requests,
     * constructs shard bulk requests and delegates execution to shard bulk action
     * */
-    private final class BulkOperation extends ActionRunnable<BulkResponse> {


I think the main difference is exception handling. I see us handling exceptions in some parts but not all. I worry about some validation or other exception inside the last part of the method. One specific example is the case where we set a parent task on the bulkShardRequest and then do client.executeLocally. I believe executeLocally can fail with a task cancelled exception.

I do think that is correctly handled everywhere though. It is mostly a matter of taste. I am inclined to prefer to let the object handle it's exceptions itself because it is passed a listener in constructor. I find it slightly confusing to have a method/object that accepts a listener but then can also throw exceptions.

That said, it is a minor point/taste matter and I am OK to keep this as is, though I would ask to add a comment to performBulkRequests that it might throw exceptions for the caller to handle and delegate to appropriate listener.

henningandersen · 2021-10-14T09:05:27Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

+         * This is called on the Transport tread so we can check the indexing
+         * memory pressure *quickly* but we don't want to keep the transport
+         * thread busy. So as son as we have the indexing pressure in we fork
+         * to one of the write thread pools.


Perhaps mention the two known reasons for this to be expensive too: tsdb parse routing from source and compression for outgoing requests?

henningandersen · 2021-10-14T09:07:58Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

@@ -462,28 +468,32 @@ protected void doRun() {
                            Version indexCreated = indexMetadata.getCreationVersion();
                            indexRequest.resolveRouting(metadata);
                            indexRequest.process(indexCreated, mappingMd, concreteIndex.getName());
+                            shardId = indexRouting.indexShard(


I am ok to keep it as is for now, but happy to hear your more elaborate thoughts on the pros and cons of this?

henningandersen · 2021-10-14T09:18:46Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

+
+                private void dispatchRetry() {
+                    /*
+                     * This is called on the cluster state update and timer


I think ClusterStateObserver uses generic thread pool for timeout. No need to change the code, but perhaps update the comment.

I see it!

notifyTimeout.cancellable = threadPool.schedule(notifyTimeout, timeout, ThreadPool.Names.GENERIC);

I'll update the comment.

henningandersen · 2021-10-14T09:26:42Z

server/src/main/java/org/elasticsearch/action/update/TransportUpdateAction.java

+        ShardIterator shardIterator = RoutingTable.shardRoutingTable(clusterState.routingTable().index(request.concreteIndex()), shardId)
+            .shardsIt();
        ShardRouting shard;
        while ((shard = shardIterator.nextOrNull()) != null) {
            if (shard.primary()) {


AFAICS, this can now be simplified to (also replacing the next 4 lines):

Suggested change

ShardIterator shardIterator = RoutingTable.shardRoutingTable(clusterState.routingTable().index(request.concreteIndex()), shardId)

.shardsIt();

ShardRouting shard;

while ((shard = shardIterator.nextOrNull()) != null) {

if (shard.primary()) {

return RoutingTable.shardRoutingTable(clusterState.routingTable().index(request.concreteIndex()), shardId)

.primaryShardIt();

👍

I see how it's using a utility method. I'm not used to these parts of the code so I hadn't realized it was there. But got it!

henningandersen · 2021-10-14T09:36:40Z

server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java

@@ -59,6 +59,7 @@
            IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING,
            IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING,
            IndexMetadata.INDEX_ROUTING_PARTITION_SIZE_SETTING,
+            IndexMetadata.INDEX_ROUTING_PATH,


Given that this is now tsdb-only, I wonder if adding it here should be guarded by the feature flag, i.e., this should move to where we also add the tsdb-mode setting? Otherwise, I think setting it is possible?

You are right I should move it under the feature flag.

henningandersen · 2021-10-14T09:42:06Z

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

@@ -329,6 +329,14 @@ public static APIBlock readFrom(StreamInput input) throws IOException {
    public static final Setting<Integer> INDEX_FORMAT_SETTING =
            Setting.intSetting(INDEX_FORMAT, 0, Setting.Property.IndexScope, Setting.Property.Final);

+    public static final Setting<List<String>> INDEX_ROUTING_PATH = Setting.listSetting(


I wonder if we need a validation here, much like we do for the mode setting? To ensure this can only be set when index-mode is tsdb. Perhaps our settings infra catches this though, if you have a test validating that, then disregard this.

Yeah, I have a test for it. The validation is in IndexMode. Instead of "routing_path must be empty in standard mode and have values in time_series mode" it is "standard mode forbids a routing_path and time series mode requires it". Just flipped.

I had a look at moving it here and didn't really like how it turned out. Most of the contents of IndexMode is validation at the moment so this validation feels right at home.

henningandersen · 2021-10-14T10:17:28Z

One more detail, this touches code where it likely makes sense to get a few extra CI runs for safety, either locally or by provoking them on this PR.

imotov

LGTM

nik9000 · 2021-10-15T14:38:40Z

One more detail, this touches code where it likely makes sense to get a few extra CI runs for safety, either locally or by provoking them on this PR.

👍

I'll provoke them on this PR. I have a lovely local machine to run the tests but its not as fast as the half dozen machines CI throws at it.

nik9000 · 2021-10-15T16:23:44Z

@elasticmachine test this please

nik9000 · 2021-10-15T17:56:57Z

@elasticmachine test this please

nik9000 · 2021-10-15T17:57:14Z

Tests are passing, but I want to run them a few more times out of paranoia.

nik9000 · 2021-10-15T20:10:39Z

@elasticmachine test this please

* upstream/master: (109 commits) Migrate custom role providers to licensed feature (elastic#79127) Remove stale AwaitsFix in InternalEngineTests (elastic#79323) Fix errors in RefreshListenersTests (elastic#79324) Reeable BwC Tests after elastic#79318 (elastic#79320) Mute BwC Tests for elastic#79318 (elastic#79319) Reenable BwC Tests after elastic#79308 (elastic#79313) Disable BwC Tests for elastic#79308 (elastic#79310) Adjust BWC for node-level field cap requests (elastic#79301) Allow total memory to be overridden (elastic#78750) Fix SnapshotBasedIndexRecoveryIT#testRecoveryIsCancelledAfterDeletingTheIndex (elastic#79269) Disable BWC tests Mute GeoIpDownloaderCliIT.testStartWithNoDatabases (elastic#79299) Add alias support to fleet search API (elastic#79285) Create a coordinating node level reader for tsdb (elastic#79197) Route documents to the correct shards in tsdb (elastic#77731) Inject migrate action regardless of allocate action (elastic#79090) Migrate to data tiers should always ensure a TIER_PREFERENCE is set (elastic#79100) Skip building of BWC distributions when building release artifacts (elastic#79180) Default ENFORCE_DEFAULT_TIER_PREFERENCE to true (elastic#79275) Deprecation of transient cluster settings (elastic#78794) ... # Conflicts: # server/src/main/java/org/elasticsearch/index/IndexMode.java # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

* upstream/master: (521 commits) Migrate custom role providers to licensed feature (elastic#79127) Remove stale AwaitsFix in InternalEngineTests (elastic#79323) Fix errors in RefreshListenersTests (elastic#79324) Reeable BwC Tests after elastic#79318 (elastic#79320) Mute BwC Tests for elastic#79318 (elastic#79319) Reenable BwC Tests after elastic#79308 (elastic#79313) Disable BwC Tests for elastic#79308 (elastic#79310) Adjust BWC for node-level field cap requests (elastic#79301) Allow total memory to be overridden (elastic#78750) Fix SnapshotBasedIndexRecoveryIT#testRecoveryIsCancelledAfterDeletingTheIndex (elastic#79269) Disable BWC tests Mute GeoIpDownloaderCliIT.testStartWithNoDatabases (elastic#79299) Add alias support to fleet search API (elastic#79285) Create a coordinating node level reader for tsdb (elastic#79197) Route documents to the correct shards in tsdb (elastic#77731) Inject migrate action regardless of allocate action (elastic#79090) Migrate to data tiers should always ensure a TIER_PREFERENCE is set (elastic#79100) Skip building of BWC distributions when building release artifacts (elastic#79180) Default ENFORCE_DEFAULT_TIER_PREFERENCE to true (elastic#79275) Deprecation of transient cluster settings (elastic#78794) ... # Conflicts: # rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/10_settings.yml # server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java # server/src/main/java/org/elasticsearch/common/settings/Setting.java # server/src/main/java/org/elasticsearch/index/IndexMode.java # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

nik9000 requested a review from henningandersen September 14, 2021 20:44

elasticsearchmachine added the v8.0.0 label Sep 14, 2021

nik9000 requested a review from imotov September 14, 2021 20:46

nik9000 added :StorageEngine/TSDB You know, for Metrics v7.16.0 labels Sep 14, 2021

Merge branch 'master' into index_routing_from_source

a6cbecf

imotov reviewed Sep 15, 2021

View reviewed changes

...multinode/src/test/resources/rest-api-spec/test/smoke_test_multinode/20_tsdb_consistency.yml Show resolved Hide resolved

build-tools-internal/src/main/groovy/elasticsearch.formatting.gradle Outdated Show resolved Hide resolved

nik9000 added 3 commits September 15, 2021 15:40

Merge branch 'master' into index_routing_from_source

8949f79

Merge branch 'master' into index_routing_from_source

8480c0b

Moar skip

9828ea0

henningandersen reviewed Sep 16, 2021

View reviewed changes

nik9000 added 5 commits September 16, 2021 10:33

tsdb survives full cluster restart

653c7f0

Remove auto generated id rejection

d762a03

We do want to reject these documents but let's sae that for a follow up change.

Simplify

85faa61

Forbid routing_required

1b398e5

Merge branch 'master' into index_routing_from_source

8c00e18

nik9000 marked this pull request as ready for review September 16, 2021 18:40

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 16, 2021

henningandersen self-requested a review September 19, 2021 13:07

nik9000 mentioned this pull request Sep 20, 2021

Add better support for metric data types (TSDB) #74660

Closed

henningandersen reviewed Sep 23, 2021

View reviewed changes

nik9000 added 3 commits September 23, 2021 11:21

Merge branch 'master' into index_routing_from_source

5f65cf2

Small

317cbe6

Fork fork knife

80548de

Let failures flow

517b2cf

henningandersen approved these changes Oct 14, 2021

View reviewed changes

imotov approved these changes Oct 15, 2021

View reviewed changes

nik9000 added 8 commits October 15, 2021 09:42

Merge branch 'master' into index_routing_from_source

d6b644d

One dispatch please

b838554

Stuff moved

b341a64

More moving

5c147d4

Explain why fork

75392e2

Back to ActionRunnable

6ff5158

Update comment

076afa5

Utility method

db1f179

nik9000 added 3 commits October 15, 2021 10:44

Move routing_path under feature flag

90afc69

Imports

63fa3bd

Merge branch 'master' into index_routing_from_source

a4c00f1

nik9000 added >non-issue auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Oct 15, 2021

elasticsearchmachine merged commit b6c61f4 into elastic:master Oct 15, 2021

fcofdez mentioned this pull request Oct 19, 2021

Fix BulkByScrollUsesAllScrollDocumentsAfterConflictsIntegTests #79428

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

wchaparro assigned nik9000 and unassigned nik9000 Dec 16, 2021

masseyke mentioned this pull request Aug 10, 2022

[CI] SplitIndexIT testSplitFromOneToN failing #88109

Closed

Route documents to the correct shards in tsdb #77731

Route documents to the correct shards in tsdb #77731

Conversation

nik9000 commented Sep 14, 2021 • edited Loading

nik9000 commented Sep 14, 2021

imotov left a comment

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 16, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Sep 23, 2021

nik9000 commented Sep 23, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen commented Oct 14, 2021

imotov left a comment

Choose a reason for hiding this comment

nik9000 commented Oct 15, 2021

nik9000 commented Oct 15, 2021

nik9000 commented Oct 15, 2021

nik9000 commented Oct 15, 2021

nik9000 commented Oct 15, 2021

nik9000 commented Sep 14, 2021 •

edited

Loading