Rename existing circuit breakers for clarity

[lanerjo]

Existing circuit breakers should be more appropriately named. eg.

• indices.breaker.total.use_real_memory -> breaker.parent.total.use_real_memory
• indices.breaker.total.limit -> breaker.parent.total.limit
• indices.breaker.request.limit -> breaker.request.limit (unless this is truly a per index breaker)
• indices.breaker.request.overhead -> breaker.request.overhead
• indices.breaker.accounting.limit -> breaker.memory.accounting.limit (unless this is per index)
• indices.breaker.accounting.overhead -> breaker.memory.accounting.overhead

[elasticmachine]

Pinging @elastic/es-core-infra (:Core/Infra/Circuit Breakers)

[soarescaique]

Hey guys, Can I get this issue resolved?

[rjernst]

@lanerjo We appreciate your desire to improve the naming of circuit breaker settings. Naming is important; good names can ease configuration, and bad names can confuse. With that said, changing existing configuration names in Elasticsearch is not a trivial matter, it must be done with care, and we choose carefully when to undertake such changes due to the massive impact they can have. For these settings, the indices prefix is standard practice, so the prefix actually helps, rather than hurts, users to find the settings. It is possible there is still reason to make this change, but I don't see any justification in the description here, just a suggestion to remove the prefix. Could you please provide some reasoning why you think this change should be made?

[lanerjo]

The existing naming convention implies that the circuit breakers apply to indices, when that is not the case.
This is misleading and inaccurate.

I do understand that this is not a trivial matter, you need to keep backwards compatibility for a period of time, to ensure that users have a chance to migrate to the new settings, and that a decision such as this should not be taken lightly.

The circuit breakers can be tripped on requests that do not include indices, such as cluster state, and inter cluster communication. Thus this naming convention can be very misleading and compounding to an already difficult to diagnose scenario.

[gwbrown]

We discussed this earlier today and came to the conclusion that we are not going to dedicate time and effort to making this change at this time.

While we agree that the existing setting names are not ideal, we have to consider the effort involved in changing them on our part, and perhaps more importantly, the effort required of our users to change their configuration and any automation they may have in place around these settings.

To give some perspective on what making this change would entail, we would have to:

1. Implement new settings with the new names, with fallbacks to the old settings,
2. Implement tests to ensure that both the new and old settings work as expected,
3. Deprecate the old settings, including implementing checks for the Deprecation Info API,
4. Update all documentation around these settings, plus create new documentation explaining why the change is being made,
5. In master only, remove the old settings.

Between this and the effort that would be required from our users, we have decided that the effort required to make this change could be used to greater benefit to our users elsewhere, and thus we have no immediate plans to make this change. If you (or another contributor) disagrees and wishes to put in the time and effort required for all of the steps above, we may accept the contribution. But as we have no plans to schedule this work on the core Elasticsearch team in the foreseeable future, I'm going to close this issue.

[lanerjo]

@gwbrown You acknowledge that the names are not ideal, and decide that there is too much work involved to clarify this for your users, and not to rectify setting names that are misleading.

Yet, changing a setting such as node.processors to processors which would've involved similar if not the same LOE.
Or changing settings from discovery.zen.* to discovery.* ?

Then you state that the level of effort for your users that would be required citing configuration and automation?

• This is something that they have to (or should) perform with every upgrade

This explanation and logic for choosing to not implement something that would benefit the community, does not make sense to me.

[gwbrown]

There are a lot of features we can implement, bugs we can fix, and setting names we can change that would all be of benefit to the community - more than we will ever have the bandwidth to do. By closing this issue, we are not saying that we don't think this change would be a good idea. We're simply stating that we have chosen to invest the development bandwidth into items we believe will bring even more benefit to the community.

What we decide to work on (and what not to work on) is always going to depend on a number of factors. Even changes which look similar from an outside perspective almost always have differing tradeoffs. For example, many of the changes to the discovery.* settings namespace were made as part of a large overhaul of the code in that area when we changed our coordination and discovery layer - since we were already making significant changes to the code, including which settings exist and what they do, changes to those settings came naturally. As such, we may decide to handle similar requests differently depending on the circumstances at the time we make the decision. Our priority is to communicate our intent clearly to our community, and closing issues we do not intend to actively work on is one way we do that.

I hope this clears up your confusion on the topic.

[jasontedor]

And processors was renamed to node.processors so that we could require that all settings be in a namespace, which we did so that we could rely on all settings names having at least one dot in them, which is important for our Docker container entrypoint to assume a convention that if an environment variable contains a dot, then it should be converted to an Elasticsearch setting (x.y -> -E x.y):

elasticsearch/distribution/src/bin/elasticsearch-env

Lines 106 to 126 in 244fc95

	# Parse Docker env vars to customize Elasticsearch
	#
	# e.g. Setting the env var cluster.name=testcluster
	#
	# will cause Elasticsearch to be invoked with -Ecluster.name=testcluster
	#
	# see https://www.elastic.co/guide/en/elasticsearch/reference/current/settings.html#_setting_default_settings
	declare -a es_arg_array
	while IFS='=' read -r envvar_key envvar_value
	do
	# Elasticsearch settings need to have at least two dot separated lowercase
	# words, e.g. `cluster.name`
	if [[ "$envvar_key" =~ ^[a-z0-9_]+\.[a-z0-9_]+ ]]; then
	if [[ ! -z $envvar_value ]]; then
	es_opt="-E${envvar_key}=${envvar_value}"
	es_arg_array+=("${es_opt}")
	fi
	fi
	done < <(env)

That is, there were more compelling reasons to make this change that brought it over the hurdles that @gwbrown mentioned.