Add ability to associate an ID with tasks #27764

imotov · 2017-12-11T22:26:26Z

Adds support for capturing the X-Opaque-Id header from a REST request and storing it's value in the tasks that this request started. It works for all user-initiated tasks (not only search).

Closes #23250

Usage:

$ curl -H "X-Opaque-Id: imotov" -H "foo:bar" "localhost:9200/_tasks?pretty&group_by=parents"
{
  "tasks" : {
    "7qrTVbiDQKiZfubUP7DPkg:6998" : {
      "node" : "7qrTVbiDQKiZfubUP7DPkg",
      "id" : 6998,
      "type" : "transport",
      "action" : "cluster:monitor/tasks/lists",
      "start_time_in_millis" : 1513029940042,
      "running_time_in_nanos" : 266794,
      "cancellable" : false,
      "headers" : {
        "X-Opaque-Id" : "imotov"
      },
      "children" : [
        {
          "node" : "V-PuCjPhRp2ryuEsNw6V1g",
          "id" : 6088,
          "type" : "netty",
          "action" : "cluster:monitor/tasks/lists[n]",
          "start_time_in_millis" : 1513029940043,
          "running_time_in_nanos" : 67785,
          "cancellable" : false,
          "parent_task_id" : "7qrTVbiDQKiZfubUP7DPkg:6998",
          "headers" : {
            "X-Opaque-Id" : "imotov"
          }
        },
        {
          "node" : "7qrTVbiDQKiZfubUP7DPkg",
          "id" : 6999,
          "type" : "direct",
          "action" : "cluster:monitor/tasks/lists[n]",
          "start_time_in_millis" : 1513029940043,
          "running_time_in_nanos" : 98754,
          "cancellable" : false,
          "parent_task_id" : "7qrTVbiDQKiZfubUP7DPkg:6998",
          "headers" : {
            "X-Opaque-Id" : "imotov"
          }
        }
      ]
    }
  }
}

mattweber · 2017-12-11T22:31:50Z

+1

I could definitely use this feature!

s1monw

this looks reasonable @imotov lets make it reviewable ie. move it out of PoC mode and we take it from there?

Adds support for capturing the X-Opaque-Id header from a REST request and storing it's value in the tasks that this request started. It works for all user-initiated tasks (not only search). Closes elastic#23250

imotov · 2017-12-21T03:04:01Z

Added pluggable headers, tests and documentation.

s1monw · 2018-01-08T14:13:49Z

@imotov I just saw this that you updated it. Sorry. I will do a review now but it needs a merge with master.

s1monw

this LGTM I left one question / suggestion

s1monw · 2018-01-08T14:20:53Z

core/src/main/java/org/elasticsearch/tasks/TaskManager.java

@@ -80,7 +89,15 @@ public void setTaskResultsService(TaskResultsService taskResultsService) {
     * Returns the task manager tracked task or null if the task doesn't support the task manager
     */
    public Task register(String type, String action, TaskAwareRequest request) {
-        Task task = request.createTask(taskIdGenerator.incrementAndGet(), type, action, request.getParentTask());
+        Map<String, String> headers = new HashMap<>();


even though we have http.max_header_size I wonder if we should also add a hard limit to the total size here. We can simply accumulate the size of the keys and values for this and fail hard if is exceeded. I really don't want to hold on to MB of strings here that are literally user input. It can be simply a derivative of http.max_header_size?

Makes sense! I am going to resolve conflicts and add this check.

…-task-id

imotov · 2018-01-08T21:01:55Z

@s1monw I merged in master, resolved conflicts and pushed check for the headers size.

nik9000 · 2018-01-09T15:06:17Z

core/src/main/java/org/elasticsearch/tasks/TaskManager.java


 /**
 * Task Manager service for keeping track of currently running tasks on the nodes
 */
 public class TaskManager extends AbstractComponent implements ClusterStateApplier {
    private static final TimeValue WAIT_FOR_COMPLETION_POLL = timeValueMillis(100);

+    /** Rest headers that are copied to the task */
+    private final Set<String> taskHeaders;


I feel like we iterate this Set a lot so we'd be better off making sure it isn't a HashSet which requires iteration of the buckets. Premature optimization, I know, but I'd feel comfortable if we copied it to a List here.

imotov · 2018-01-10T21:31:10Z

@s1monw do you want to take another look before I merge this in? Also should this be 7.0-only or should I try back-porting this to 6.x? What do you think?

s1monw

LGTM

…-task-id

nik9000 · 2018-01-12T19:57:13Z

Also should this be 7.0-only or should I try back-porting this to 6.x? What do you think?

It is a little breaking for the plugin API but not very. I think it'd be fine to backport.

* master: (30 commits) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (elastic#28041) Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (elastic#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (elastic#28019) Ignore null value for range field (elastic#27845) (elastic#28116) Fix environment variable substitutions in list setting (elastic#28106) ...

* master: (59 commits) Correct backport replica rollback to 6.2 (elastic#28181) Backport replica rollback to 6.2 (elastic#28181) Rename deleteLocalTranslog to createNewTranslog AwaitsFix #testRecoveryAfterPrimaryPromotion TEST: init unassigned gcp in testAcquireIndexCommit Replica start peer recovery with safe commit (elastic#28181) Truncate tlog cli should assign global checkpoint (elastic#28192) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc ...

Adds support for capturing the X-Opaque-Id header from a REST request and storing it's value in the tasks that this request started. It works for all user-initiated tasks (not only search). Closes #23250 Usage: ``` $ curl -H "X-Opaque-Id: imotov" -H "foo:bar" "localhost:9200/_tasks?pretty&group_by=parents" { "tasks" : { "7qrTVbiDQKiZfubUP7DPkg:6998" : { "node" : "7qrTVbiDQKiZfubUP7DPkg", "id" : 6998, "type" : "transport", "action" : "cluster:monitor/tasks/lists", "start_time_in_millis" : 1513029940042, "running_time_in_nanos" : 266794, "cancellable" : false, "headers" : { "X-Opaque-Id" : "imotov" }, "children" : [ { "node" : "V-PuCjPhRp2ryuEsNw6V1g", "id" : 6088, "type" : "netty", "action" : "cluster:monitor/tasks/lists[n]", "start_time_in_millis" : 1513029940043, "running_time_in_nanos" : 67785, "cancellable" : false, "parent_task_id" : "7qrTVbiDQKiZfubUP7DPkg:6998", "headers" : { "X-Opaque-Id" : "imotov" } }, { "node" : "7qrTVbiDQKiZfubUP7DPkg", "id" : 6999, "type" : "direct", "action" : "cluster:monitor/tasks/lists[n]", "start_time_in_millis" : 1513029940043, "running_time_in_nanos" : 98754, "cancellable" : false, "parent_task_id" : "7qrTVbiDQKiZfubUP7DPkg:6998", "headers" : { "X-Opaque-Id" : "imotov" } } ] } } } ```

Update the serialization version after backporting #27764 to 6.x.

* master: (74 commits) Update version of TaskInfo header serialization after backport TEST: Tightens file-based condition in peer-recovery Correct backport replica rollback to 6.2 (elastic#28181) Backport replica rollback to 6.2 (elastic#28181) Rename deleteLocalTranslog to createNewTranslog AwaitsFix #testRecoveryAfterPrimaryPromotion TEST: init unassigned gcp in testAcquireIndexCommit Replica start peer recovery with safe commit (elastic#28181) Truncate tlog cli should assign global checkpoint (elastic#28192) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads ...

Mpdreamz · 2018-01-15T18:46:19Z

Super late to this but is there a particular reason for this being a request header and not a common query string parameter? I would personally much rather settle on the latter.

imotov · 2018-01-16T14:34:53Z

@Mpdreamz it was the most straightforward way to implement it internally across all requests in a transparent way. It also seems different in the nature to common request parameters, that change the output format. In our case we just mark the request, which is more similar to authentication or "run-as" headers then to the common format parameters.

* compile-with-jdk-9: (56 commits) TEST: init unassigned gcp in testAcquireIndexCommit Replica start peer recovery with safe commit (elastic#28181) Truncate tlog cli should assign global checkpoint (elastic#28192) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (elastic#28041) Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (elastic#28174) ...

codebrain · 2018-06-07T03:14:45Z

+1 to @Mpdreamz sentiment around this being preferred as a querystring parameter.

Implemented in the .NET client - https://github.com/elastic/elasticsearch-net/pull/3277/files

As far as I can see this parameter is only valid on certain user-initiated actions, yet the parameter value is set at the request headers level? In the .NET client this means that the value can theoretically be set on any API call - which may start to mislead client users. I can see the justification for the RunAs parameter, but this one does look different in scope.

Using the parameter in the headers collection as a filter to the task lists is also a little non-obvious and ought to be on the querystring call to tasks list, imo.

imotov · 2018-06-07T13:54:21Z

As far as I can see this parameter is only valid on certain user-initiated actions

I am not totally sure what you mean by this. Could you elaborate?

codebrain · 2018-06-08T00:05:55Z

As far as I can tell this parameter only makes sense on API calls where there are potentials for long running tasks? i.e. if a client set this parameter (unique id) on every API call, would every API call result in a task in the task management API?

imotov · 2018-06-08T18:26:47Z

As far as I can tell this parameter only makes sense on API calls where there are potentials for long running tasks?

That's the main use case, but nobody prevents you from using it on every call since sometimes it's not very clear how long a call will take and definition of "long" is somewhat vague.

if a client set this parameter (unique id) on every API call, would every API call result in a task in the task management API?

Every API call (including internal API calls) result in a task in the task manager regardless. Even if you perform a call to list tasks in the task manager you will see this call in the list of returned tasks.

codebrain · 2018-06-12T01:04:54Z

Thanks for clarifying @imotov

imotov added :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. WIP labels Dec 11, 2017

imotov requested a review from s1monw December 11, 2017 22:26

s1monw reviewed Dec 14, 2017

View reviewed changes

Add adding ability to associate an ID with tasks

ff8a32c

Adds support for capturing the X-Opaque-Id header from a REST request and storing it's value in the tasks that this request started. It works for all user-initiated tasks (not only search). Closes elastic#23250

imotov force-pushed the issue-23250-search-task-id branch from 3ad0abe to ff8a32c Compare December 21, 2017 03:00

imotov changed the title ~~POC for adding ability to associate a search task ID~~ Add adding ability to associate an ID with tasks Dec 21, 2017

imotov added review and removed WIP labels Dec 21, 2017

imotov requested a review from nik9000 December 21, 2017 03:04

Fix rolling upgrade test

54df342

s1monw reviewed Jan 8, 2018

View reviewed changes

imotov added 3 commits January 8, 2018 12:21

Merge remote-tracking branch 'elastic/master' into issue-23250-search…

f717dda

…-task-id

Resolve issues after the merge

3a60071

Add check for the maximum headers size

085fadb

imotov changed the title ~~Add adding ability to associate an ID with tasks~~ Add ability to associate an ID with tasks Jan 9, 2018

nik9000 approved these changes Jan 9, 2018

View reviewed changes

Copy the header set into list to avoid iterating over a HashMap

e50bc6b

s1monw approved these changes Jan 10, 2018

View reviewed changes

imotov added the v7.0.0 label Jan 11, 2018

Merge remote-tracking branch 'elastic/master' into issue-23250-search…

1aa4eb4

…-task-id

imotov merged commit c75ac31 into elastic:master Jan 12, 2018

imotov added backport pending and removed review labels Jan 12, 2018

imotov added a commit that referenced this pull request Jan 15, 2018

Update version of TaskInfo header serialization after backport

aec0c0f

Update the serialization version after backporting #27764 to 6.x.

imotov added v6.2.0 >enhancement and removed backport pending labels Jan 15, 2018

This was referenced Feb 2, 2018

[Search service] Add search IDs to requests made to Elasticsearch elastic/kibana#16493

Closed

add task ID to slowlogs if X-Opaque-Id header was set #28502

Closed

elasticmachine mentioned this pull request Apr 25, 2018

add task ID to audit logging if X-Opaque-Id header was set #30052

Closed

AlonaNadler mentioned this pull request May 4, 2018

[Management] List / Terminate Running Tasks elastic/kibana#18829

Open

codebrain mentioned this pull request May 23, 2018

[6.2] Add ability to associate an ID with tasks elastic/elasticsearch-net#3259

Closed

imotov mentioned this pull request May 25, 2018

Add task list filtering by header #30868

Open

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

imotov deleted the issue-23250-search-task-id branch May 1, 2020 22:15

imotov mentioned this pull request Sep 22, 2020

Allow comment in request payload to be returned in the response payload. #61118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to associate an ID with tasks #27764

Add ability to associate an ID with tasks #27764

imotov commented Dec 11, 2017 •

edited

Loading

mattweber commented Dec 11, 2017

s1monw left a comment

imotov commented Dec 21, 2017

s1monw commented Jan 8, 2018

s1monw left a comment

s1monw Jan 8, 2018

imotov Jan 8, 2018

imotov commented Jan 8, 2018

nik9000 Jan 9, 2018

imotov commented Jan 10, 2018

s1monw left a comment

nik9000 commented Jan 12, 2018

Mpdreamz commented Jan 15, 2018

imotov commented Jan 16, 2018

codebrain commented Jun 7, 2018

imotov commented Jun 7, 2018

codebrain commented Jun 8, 2018

imotov commented Jun 8, 2018

codebrain commented Jun 12, 2018

Add ability to associate an ID with tasks #27764

Add ability to associate an ID with tasks #27764

Conversation

imotov commented Dec 11, 2017 • edited Loading

mattweber commented Dec 11, 2017

s1monw left a comment

Choose a reason for hiding this comment

imotov commented Dec 21, 2017

s1monw commented Jan 8, 2018

s1monw left a comment

Choose a reason for hiding this comment

s1monw Jan 8, 2018

Choose a reason for hiding this comment

imotov Jan 8, 2018

Choose a reason for hiding this comment

imotov commented Jan 8, 2018

nik9000 Jan 9, 2018

Choose a reason for hiding this comment

imotov commented Jan 10, 2018

s1monw left a comment

Choose a reason for hiding this comment

nik9000 commented Jan 12, 2018

Mpdreamz commented Jan 15, 2018

imotov commented Jan 16, 2018

codebrain commented Jun 7, 2018

imotov commented Jun 7, 2018

codebrain commented Jun 8, 2018

imotov commented Jun 8, 2018

codebrain commented Jun 12, 2018

imotov commented Dec 11, 2017 •

edited

Loading