Add config change handler for Bulkhead. Simplify BulkheadConfig. #64

damtur · 2017-10-10T15:33:26Z

I've added IObservable into MjolnirConfiguration.
I've made it abstract, so someone using the library may wire-up their implementation of Config change handler.
I've added NonObservableMjolnirConfiguration with no-op implementation for default config.
I've added sample wire-up using Microsoft JSON configuration.
I've added a couple test which should test change handlers using sample configuration.

robhruska · 2017-10-10T15:43:09Z

Hudl.Mjolnir/Bulkhead/BulkheadFactory.cs

@@ -45,7 +46,7 @@ public BulkheadFactory(IMetricEvents metricEvents, IBulkheadConfig bulkheadConfi
                        if (_bulkheads.TryGetValue(key, out Lazy<SemaphoreBulkheadHolder> holder) && holder.IsValueCreated)
                        {
                            var bulkhead = holder.Value.Bulkhead;
-                            _metricEvents.BulkheadGauge(bulkhead.Name, "semaphore", _bulkheadConfig.GetMaxConcurrent(key), bulkhead.CountAvailable);
+                            _metricEvents.BulkheadGauge(bulkhead.Name, "semaphore", _config.GetBulkheadConfiguration(key.Name).MaxConcurrent, bulkhead.CountAvailable);


Would it be better to pass in a GroupKey to GetBulkheadConfiguration?

I had considered that but decided that it'll be easier to implement Configuration in any adapter if that will be just the string. So for simplicity, I've left it as a string.

More of an opinion, but I think it's clearer to the calling code if it just has to pass in a GroupKey, since then callers don't have to worry about translating a GroupKey to its appropriate configuration string - the configuration code can know what to do with the GroupKey and do it consistently for any call. GroupKey is the abstraction over bulkhead/breaker identifiers and I've always preferred to use that within Mjolnir if it's available instead of raw strings.

Not a strong argument, but sort of relevant: http://wiki.c2.com/?StringlyTyped

robhruska · 2017-10-10T16:56:16Z

Hudl.Mjolnir/Config/BulkheadConfigurationObserver.cs

+        public void OnNext(BulkheadConfiguration value)
+        {
+            var newValue = _expression(value);
+            var hasChanged = !Equals(_currentValue, newValue);


Does this work, particularly for reference types? If T is a String, what does this evaluate to if they're the same value?

Given that this is Object.Equals and strings are reference types, I'm concerned this'll do an object reference comparison.

This should work as string has "Equals" overriden. From Object code:

public virtual bool Equals(object obj) { return RuntimeHelpers.Equals(this, obj); } public static bool Equals(object objA, object objB) { if (objA == objB) return true; if (objA == null || objB == null) return false; return objA.Equals(objB); }

I've also checked with my debugger and it seems to work on my machine.

Ah cool, thanks for checking.

robhruska · 2017-10-10T17:00:04Z

Hudl.Mjolnir/Config/StaticBulkheadConfiguration.cs

+namespace Hudl.Mjolnir.Config
+{
+    /// <summary>
+    /// Used only for configs which will never change.


Minor, but is this true? If observables aren't used, configuration values can still change, correct? It's just that they won't fire change events. A re-read of the configuration property may yield a different value, though, right?

In that case "Static" may be a bit misleading as the type name here. Could go with something like NonObservableBulkheadConfiguration?

For the first comment: It will never change since this is only used for DefaultMjolnirConfig which is implemented inside the library already. If someone will provide different config they have to implement their own implementation of BulkheadConfiguration.
I'll make that class internal!
Also I'll change its name.

robhruska · 2017-10-10T17:03:00Z

Hudl.Mjolnir/Config/BulkheadConfigurationObserverCreationExtensions.cs

+
+namespace Hudl.Mjolnir.Config
+{
+    internal static class BulkheadConfigurationObserverCreationExtensions


Minor: Since these are extensions on BulkheadConfiguration, I'd expect this to be named BulkheadConfigurationExtensions.

… config.

robhruska · 2017-10-16T15:23:52Z

Hudl.Mjolnir.Tests/Configuration/Helpers/test.json

@@ -0,0 +1,11 @@
+{
+    "testconfig": {
+        "isenabled": true,


Does case matter here? The case of these keys is inconsistent.

robhruska · 2017-10-16T15:25:51Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs


        /// <summary>
        /// Per-breaker configuration. breaker-key is the argument passed to the Command constructor.
        /// </summary>
-        protected Dictionary<string, BreakerConfiguration> BreakerConfigurations { get; set; }
+        public Dictionary<string, BreakerConfiguration> BreakerConfigurations { get; set; }


Should the keys for these Dictionaries be GroupKeys now that it's the argument to access config?

robhruska · 2017-10-16T15:26:30Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs

        {
-            if (key == null) return DefaultBulkheadConfiguration;
+            if (string.IsNullOrEmpty(key.Name)) return DefaultBulkheadConfiguration;


IsNullOrWhiteSpace

robhruska · 2017-10-16T15:27:25Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs

@@ -100,11 +102,11 @@ public CommandConfiguration GetCommandConfiguration(string groupKey = null)
        /// <param name="key">Bulkhead configuration for a given key. Default value returned if non-existent or 
        /// null.</param>
        /// <returns></returns>
-        public BulkheadConfiguration GetBulkheadConfiguration(string key = null)
+        public BulkheadConfiguration GetBulkheadConfiguration(GroupKey key)


Are the other methods (GetCommandConfiguration, GetBreakerConfiguration) switching over to GroupKey instead of string as well?

robhruska · 2017-10-16T15:28:59Z

Hudl.Mjolnir/Config/MjolnirConfigurationObserver.cs

+
+        public void OnError(Exception error)
+        {
+            throw new NotImplementedException();


Is this the best approach here? i.e. is it impossible for OnError to happen, or should we have better handling here?

Good find. I'll change to no-op.
FWIW With my implementation, it's not possible for OnError or OnCompleted to be fired.

robhruska · 2017-10-16T15:29:42Z

Hudl.Mjolnir/Config/MjolnirConfigurationObserver.cs

+            }
+            catch (Exception)
+            {
+                //Purely to make sure that other change events still fire 


Can this be clearer about why we're ignoring the exception? Should we at least log here?

robhruska · 2017-10-16T15:34:32Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs

+        /// </summary>
+        /// <param name="observer">Mjolnir configuration observer which is acting upon configuration change</param>
+        /// <returns></returns>
+        public abstract IDisposable Subscribe(IObserver<MjolnirConfiguration> observer);


Is it possible to have this class (MjolnirConfiguration) handle all of the observer logic and make this internal instead of public abstract? It looks like most of the implementations of this are the same - do inheriting classes need their own control over how the subscriptions work?

robhruska · 2017-10-16T15:39:55Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs

            BulkheadConfiguration bulkheadConfiguration;
-            return BulkheadConfigurations.TryGetValue(key, out bulkheadConfiguration) ?
+            return BulkheadConfigurations.TryGetValue(key.Name, out bulkheadConfiguration) ?


Is it possible for the Dictionary to have a null value for any key? It might be good to protect against returning null values from this method to prevent NullReferenceExceptions when callers access properties on the returned value, since the calling code assumes non-null objects returned.

robhruska · 2017-10-16T15:41:15Z

Hudl.Mjolnir/Config/MjolnirConfiguration.cs

+
+
+        /// <summary>
+        /// Allows subscribtions for configuration change. Whenever any property change in MjolnirConfig all 


"subscribtions"

Add config change handler for Bulkhead. Simplify BulkheadConfig.

46edf95

damtur requested a review from robhruska October 10, 2017 15:33

robhruska reviewed Oct 10, 2017

View reviewed changes

damtur added 3 commits October 10, 2017 19:57

Use GroupKey for BulkheadConfiguration. Minor classes renames.

8fffb96

Mjolnir config to supprot updates. Add Config tests. Add example json…

51bb883

… config.

Remove documentation for SnapshotTtlMillis

55a3112

damtur mentioned this pull request Oct 16, 2017

Version3 - Configuration rewrite for Mjolnir. #63

Merged

robhruska reviewed Oct 16, 2017

View reviewed changes

Make Config a public sealed class. Address few other issues.

a4dcc57

damtur merged commit 96c9906 into Version3-ConfigMigration Oct 18, 2017

damtur deleted the Version3-ConfigMigration-ChangeHandler branch October 18, 2017 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config change handler for Bulkhead. Simplify BulkheadConfig. #64

Add config change handler for Bulkhead. Simplify BulkheadConfig. #64

damtur commented Oct 10, 2017 •

edited

Loading

robhruska Oct 10, 2017

damtur Oct 10, 2017

robhruska Oct 10, 2017 •

edited

Loading

robhruska Oct 10, 2017

robhruska Oct 10, 2017

damtur Oct 10, 2017

robhruska Oct 10, 2017

robhruska Oct 10, 2017

robhruska Oct 10, 2017

damtur Oct 10, 2017 •

edited

Loading

robhruska Oct 10, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

damtur Oct 18, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017

robhruska Oct 16, 2017



		/// <summary>
		/// Allows subscribtions for configuration change. Whenever any property change in MjolnirConfig all

Add config change handler for Bulkhead. Simplify BulkheadConfig. #64

Add config change handler for Bulkhead. Simplify BulkheadConfig. #64

Conversation

damtur commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhruska Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damtur Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damtur commented Oct 10, 2017 •

edited

Loading

robhruska Oct 10, 2017 •

edited

Loading

damtur Oct 10, 2017 •

edited

Loading