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

Already on GitHub? Sign in to your account

Cuttlefish schema for background manager global kill/enable switch #497

Merged
merged 4 commits into from Feb 20, 2014

Conversation

Projects
None yet
5 participants
Contributor

buddhisthead commented Dec 31, 2013

Add cuttlefish schema for the global background manager kill/enable switch.

  • On by default
  • Advanced mode setting means it doesn't get generated into the config file "riak.config"
  • Look for it in "dev/devN/data/generated.configs" where N=1..6 after make devrel

I examined the generated.configs file to confirm it was present and had the correct default value… you should see this:

{riak_core,
     [{use_background_manager,true},
      {enable_consensus,false},
     …

@seancribbs seancribbs and 2 others commented on an outdated diff Dec 31, 2013

priv/riak_core.schema
@@ -288,3 +288,15 @@
{default, false},
{commented, true}
]}.
+
+%% @doc Enable or skip use of the background manager. When enabled, participating
+%% Riak subsystems will coordinate access to shared resources. This will help
+%% to prevent system response degradation under times of heavy load from multiple
+%% background tasks. To disable background coordination entirely, set this parameter
+%% to false. Specific subsystems may also have their own controls over use of the
+%% background manager. Riak 2.0+.
+{mapping, "use_bg_manager", "riak_core.use_background_manager", [
@seancribbs

seancribbs Dec 31, 2013

Contributor

As we say in the "confbal": "how many starving children in third-world countries were saved by omitting letters in that word"? Also, how about background_manager = enabled | disabled?

@seancribbs

seancribbs Dec 31, 2013

Contributor

Thank you for reminding me too that I need to implement a flag datatype, which amounts to an enum but lets you pick the names of each and converts them to true or false.

@buddhisthead

buddhisthead Dec 31, 2013

Contributor

I will grant you the letters from the starving children, but we had a discussion on hip chat today (so it didn't happen) about the kill switch configuration variables and how to name them. We did not burn white smoke, but I will argue that "enabled" is different from "use", especially for something like an API that is used by other subsystems. The background manager has a separate enable/disable API call that continues to track resources but behaves like there are no limits on tokens and locks. In contrast, the "use-background-manager" configuration variable is used by other subsystems to control if they even wish to call the API at all. This is the global "kill switch" that disables full use of the feature. We also observed that there is no consistent naming convention for kill switches or even whether it's "use it" versus "disable it". All of our new features do this a little differently. So, this is what I felt best described what the variable really controlled: "use it", rather than "it's turned off". Maybe we've gone too far with the kill switch concept.

AAE uses "anti_entropy = on | off | debug", for example. If the great spirits of the Cuttlefish have a unified proposal, I will sprinkle food in their tank as thanks.

@jrwest

jrwest Dec 31, 2013

Contributor

proposing a totally different approach:

The end user doesn't know what the background_manager is. How about limit_background_work or something to that effect? Unfortunately some options are probably not available to us because they would require changing the actual handling of the options in the code, but just a thought.

@seancribbs

seancribbs Dec 31, 2013

Contributor

@jrwest I like your approach.

@buddhisthead My point was mostly that I've seen a bunch of settings land that we end up renaming later because they don't make sense to people who aren't us ("we are the enemy of the user"). This is an advanced setting, yes, but if someone found it in riak.conf, how much context would they have in its meaning?

Naming things is hard.

@buddhisthead

buddhisthead Dec 31, 2013

Contributor

This switch is only here as a fail-safe, which would probably only get changed at the recommendation of a CSE. So, it should be named something that a CSE would be comfortable with. Also, I think we need to consider the name in the context of all the other kill switches. I'd rather us have a consistent naming convention for all of our kill switches. Then, it makes sense to change this setting's name.

@jrwest I do like your idea. If we change this setting, then we should probably change the KV switches that use bg-mgr as well. See basho/riak_kv#793. In their case, "limit background work" would be confusing since they already have their own concurrency controls. It's more about "sharing" in those cases. I think the naming should reveal the connection between all three of these settings.

@jrwest

jrwest Dec 31, 2013

Contributor

partition_background_limit?

@buddhisthead

buddhisthead Dec 31, 2013

Contributor

For the background manager, we need a setting name that will make sense for other things besides partitions, such as token rates and locks that we eventually back-port to existing code. I do like the limit_concurrent_partition_work for the subsystems though. E.g. anti_entropy.limit_concurrent_partition_work and mdc.fullsync.limit_concurrent_partition_work. Substitute the word fold for work to be even more exact.

Contributor

seancribbs commented Dec 31, 2013

@buddhisthead No disagreement about consistency. We can also add @see annotations that refer to the global setting to reduce ambiguity.

Contributor

seancribbs commented Jan 31, 2014

  • CSE approval
  • PM approval
  • Eng testing
Contributor

joedevivo commented Jan 31, 2014

Looks good. +1 for tests passing and consistency with the other PR in riak_kv

Contributor

seancribbs commented Jan 31, 2014

Ugh, going to rework that doc string.

Contributor

seancribbs commented Feb 5, 2014

@jmshoffs0812 @angrycub @mk1s Need a CSE +1

+1

seancribbs added a commit that referenced this pull request Feb 20, 2014

Merge pull request #497 from basho/cet-bg-mgr-schema
Cuttlefish schema for background manager global kill/enable switch

@seancribbs seancribbs merged commit 9dadb32 into develop Feb 20, 2014

1 check failed

default Eunit done.
Details

@seancribbs seancribbs deleted the cet-bg-mgr-schema branch Feb 20, 2014

@jrwest jrwest modified the milestones: 2.0-beta, 2.0 Mar 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment