[CCR] Add total fetch time took stat #34577
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
keeps track how much time was spent on fetches from the leader cluster perspective.
martijnvg
added
>non-issue
WIP
:Distributed Indexing/CCR
|
Pinging @elastic/es-distributed |
|
I think I'm good with the following:
|
|
yes, this pr keeps the original total_fetch_time_millis and adds a took
time field to the shard changes api response. I will rename
total_fetch_took_time_millis to total_fetch_leader_time_millis .
…On Thu, Oct 18, 2018 at 16:38 Boaz Leskes ***@***.***> wrote:
I think I'm good with the following:
1. Changes response has a took time field
2. Shard Follow Node Task sums the value of that feed under
total_fetch_leader_time_millis
3. We have a total_fetch_time_millis as we do today.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#34577 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjbRXSuFRdq-nFYYflgBkrVwWdxVh6Zks5umJJrgaJpZM4Xs2D6>
.
|
jasontedor
requested changes
Oct 18, 2018
| } | ||
|
|
||
| @Override | ||
| protected void asyncShardOperation( | ||
| final Request request, | ||
| final ShardId shardId, | ||
| final ActionListener<Response> listener) throws IOException { | ||
| request.relativeStartNanos = System.nanoTime(); |
There was a problem hiding this comment.
I think this is the wrong place to start the clock. The goal with this metric is to capture the non-network time on the leader so we want to start the clock as soon as possible. Right now these requests are handled on the network thread before we go async, so there is not much in terms of queuing time, but it would be a bug if somehow this ever changed and handling of this request was not on the network thread before going async, this would miss queuing time in that other thread pool (we miss internal Netty queuing, but I don't think there's anything obvious that we can do about that). This is also something to think about in the context of the NIO work. I also don't like that we are mutating the request, it's doing to make it harder to have immutable request objects.
I think we should start the clock the moment the request is de-serialized.
|
@bleskes @jasontedor I've updated this PR. |
martijnvg
added a commit
that referenced
this pull request
Oct 23, 2018
Add total fetch time leader stat, that keeps track how much time was spent on fetches from the leader cluster perspective.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 23, 2018
* master: (24 commits) ingest: better support for conditionals with simulate?verbose (elastic#34155) [Rollup] Job deletion should be invoked on the allocated task (elastic#34574) [DOCS] .Security index is never auto created (elastic#34589) CCR: Requires soft-deletes on the follower (elastic#34725) re-enable bwc tests (elastic#34743) Empty GetAliases authorization fix (elastic#34444) INGEST: Document Processor Conditional (elastic#33388) [CCR] Add total fetch time leader stat (elastic#34577) SQL: Support pattern against compatible indices (elastic#34718) [CCR] Auto follow pattern APIs adjustments (elastic#34518) [Test] Remove dead code from ExceptionSerializationTests (elastic#34713) A small typo in migration-assistance doc (elastic#34704) ingest: processor stats (elastic#34724) SQL: Implement IN(value1, value2, ...) expression. (elastic#34581) Tests: Add checks to GeoDistanceQueryBuilderTests (elastic#34273) INGEST: Rename Pipeline Processor Param. (elastic#34733) Core: Move IndexNameExpressionResolver to java time (elastic#34507) [DOCS] Force Merge: clarify execution and storage requirements (elastic#33882) TESTING.asciidoc fix examples using forbidden annotation (elastic#34515) SQL: Implement `CONVERT`, an alternative to `CAST` (elastic#34660) ...
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
Add total fetch time leader stat, that keeps track how much time was spent on fetches from the leader cluster perspective.
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.
This new stat keeps track how much time was spent on fetches from the leader cluster perspective.
The current name of this new stat is
total_fetch_took_time_millisand it doesn't describe well what this stat represents. I'm not sure what would be a good name. Maybetotal_fetch_leader_time_millisortotal_fetch_remote_time_millisto indicate it is the same astotal_fetch_time_millisbut from a different perspective? cc: @bleskes / @jasontedor