Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cluster Health: Add wait time for pending task and recovery percentage #11393

Conversation

spinscale
Copy link
Contributor

In order to get a quick overview using by simply checking the cluster state
and its corresponding cat API, the following two attributes have been added
to the cluster health response:

  • pending_task_time_in_queue, the time value of the first task of the
    queue and how long it has been waiting
  • recovery percent: The percentage of the number of shards that are in
    initializing state

This makes the cluster health API handy to check, when a fully restarted
cluster is back up and running.

In addition a small serialization fix has been added, which removes version
checks for the this branch in the ClusterHealthResponse.

Closes #10805

@spinscale
Copy link
Contributor Author

biggest question here is, if the shardRouting.initialized() call is sufficient as an information, that this shard is being recovered.. if not this cannot be implemented as part of the cluster health being a master only operation, as we need to get shard information and the current RecoveryState

@clintongormley clintongormley added the :Data Management/Stats Statistics tracking and retrieval APIs label May 28, 2015
@spinscale
Copy link
Contributor Author

@clintongormley care to take a look, if this matches your expectation, even though no new cat API has been added?

@clintongormley
Copy link

LGTM

int initializingShardCount = 0;
int totalShardCount = 0;
for (ShardRouting shardRouting : shardRoutings) {
if (shardRouting.initializing()) initializingShardCount++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should count shardRouting.active() == false no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not following here, why?

@bleskes
Copy link
Contributor

bleskes commented Jun 3, 2015

Hey @spinscale, left some comment about naming and implementation. I think we comments also address the safe use of shardRouting.initialized() - I think we should make it a master only API, no reaching out to the individual nodes. If we want to have an aggregation of the _recovery info, we should add it as a header there imho.

@spinscale
Copy link
Contributor Author

@bleskes so, you are proposing two important changes here, which I want to understand before applying them

  1. pending_task_waiting should become the longest waiting time in the task list. This means, that when the cluster continouosly accepts new pending tasks with higher priority we do not see a change here. However, as one can also see the number of pending tasks and those change, this might be ok. What worries my about the approach of scanning all tasks, is that the speed of the cluster health response is dependent on the number of tasks in the queue. Slowing down this API doesnt sound like a good idea.
  2. recovery_percent should become active_percent. Sounds good. The current metric is pretty useless, as the number of concurrent recoveries per node (2 by default iirc) and the number of total shards is going to be static. Something I hadnt thought about yet, is how unassigned shards come into play here. But I dont think it is a big problem that the active percentage is 50%, when you have a single node and one replica for your indices.

@bleskes
Copy link
Contributor

bleskes commented Jun 8, 2015

What worries my about the approach of scanning all tasks, is that the speed of the cluster health response is dependent on the number of tasks in the queue.

That was my concern as well when thinking about this but I decided it's worth it. I think it's more power full this way. It's only an array scan so we're still talking super fast, even if it's 10K ops.

But I dont think it is a big problem that the active percentage is 50%, when you have a single node and one replica for your indices.

Yeah, I think it communicates clearly how bad is "YELLOW"

@spinscale spinscale force-pushed the 1505-recovery-progress-cluster-health-issue-10805 branch from d6c104e to d3a3651 Compare June 15, 2015 14:40
@spinscale
Copy link
Contributor Author

upgraded the PR with all the naming refactoring and scanning all the tasks (also introduced a HasCreationDate interface to make it simpler to read the creation date).

} else {
List<ShardRouting> shardRoutings = clusterState.getRoutingTable().allShards();
// shortcut on no shards
if (shardRoutings.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be covered by the green case, no?

@bleskes
Copy link
Contributor

bleskes commented Jun 16, 2015

I like this! one thing I would love seeing is getting rid of org.elasticsearch.cluster.service.InternalClusterService.TimedPrioritizedRunnable - the time aspect should be folded into the executor..

@spinscale spinscale force-pushed the 1505-recovery-progress-cluster-health-issue-10805 branch from d3a3651 to 61981d8 Compare June 17, 2015 12:43
@spinscale
Copy link
Contributor Author

@bleskes thx for the comments, folded them in, also removed the TimedPrioritizedRunnable

}
}

return TimeValue.timeValueNanos(System.nanoTime() - oldestCreationDateInNanos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit picky - Can we capture the initial long oldestCreationDateInNanos = System.nanoTime(); and use this as the "now"? Just worried that the queue can be empty (after the initial check) and we will still get a non 0 value ..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid. fixed

@bleskes
Copy link
Contributor

bleskes commented Jun 19, 2015

Went through it and it looks good! I mis a non-cat rest test. If you agreed with the comments, feel free to push this - no need for another round.

I also wonder if we should bite the bullet and add active_primary_shards_percent (since we have active_primary_shards )

@spinscale
Copy link
Contributor Author

I'll add another test and check the renaming as well. will push upon running tests. Thanks for reviewing!

@spinscale spinscale force-pushed the 1505-recovery-progress-cluster-health-issue-10805 branch 3 times, most recently from 1669d42 to c85557e Compare June 22, 2015 13:02
In order to get a quick overview using by simply checking the cluster state
and its corresponding cat API, the following two attributes have been added
to the cluster health response:

* task max waiting time, the time value of the first task of the
  queue and how long it has been waiting
* active shards percent: The percentage of the number of shards that are in
  initializing state

This makes the cluster health API handy to check, when a fully restarted
cluster is back up and running.

Closes elastic#10805
@spinscale
Copy link
Contributor Author

closed by 88f8d58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Stats Statistics tracking and retrieval APIs >feature v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add recovery progress (% recovered) to the cat API
4 participants