Add ingest_took to bulk response
#16876
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martijnvg
added
>enhancement
v5.0.0-alpha1
:Data Management/Ingest Node
| @@ -101,6 +101,7 @@ void processIndexRequest(Task task, String action, ActionListener listener, Acti | |||
| } | |||
|
|
|||
| void processBulkIndexRequest(Task task, BulkRequest original, String action, ActionFilterChain chain, ActionListener<BulkResponse> listener) { | |||
| long ingestStartTime = System.currentTimeMillis(); | |||
There was a problem hiding this comment.
The reason I picked System.currentTimeMillis() is because that is what the bulk api uses too. Otherwise that would be inconsistent?
There was a problem hiding this comment.
any time we measure deltas (how long something took) we should use nanoTime. currentTimeMillis can be reset by ntp ... if the bulk api uses it for this purpose, it should be fixed...
There was a problem hiding this comment.
don't we have millis everywhere else? isn't it better to keep millis here too?
There was a problem hiding this comment.
sorry I commented before reading your answer... ignore me
There was a problem hiding this comment.
Makes sense, I can fix the bulk api to use nanoTime instead of currentTimeMillis in a separate PR.
| @@ -106,6 +126,7 @@ public void readFrom(StreamInput in) throws IOException { | |||
| responses[i] = BulkItemResponse.readBulkItem(in); | |||
| } | |||
| tookInMillis = in.readVLong(); | |||
| ingestTookInMillis = in.readLong(); | |||
There was a problem hiding this comment.
you can sue ZLong for small values (FYI)...
There was a problem hiding this comment.
nice, I didn't know this existed.
martijnvg
force-pushed
the
ingest_took
branch
2 times, most recently
from
a9eb4e5
to
1016139
Compare
|
LGTM |
…ent on ingest preprocessing. The `ingest_took` is separate from `took`, which keeps track how much time is spent on indexing/deleting/updating. The `ingest_took` is only visible in the rest response if at least for one bulk item has ingest enabled.
|
this PR passes invalid tookInMillis values to serialization. https://github.com/elastic/elasticsearch/pull/16876/files#diff-764ab3948584e67f5cb1d566ef82186aR38 -1 is not allowed for vlong it must be positive. I have a failing test because of this: |
|
argh scratch that - I misread the line... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ingest_tookindicates how much time was spent on ingest preprocessing.The
ingest_tookis separate fromtook, which keeps track how much time is spent on indexing/deleting/updating. Theingest_tookis only visible in the rest response if at least one index request has ingest enabled.