Define dynamic config and integrate in service bootstrap

[madhuravi]

Setting up dynamic config and enable for a few config parameters.

[mfateev]

I thought we agreed on the design that doesn't use lookups to config from the application code. Instead we only use structures to configure any application logic.

[madhuravi]

@mfateev Similar to latest commit? Or ware you implying dependency injection?

[coveralls]

Coverage decreased (-0.006%) to 67.513% when pulling 6ec4a4d on dynamic into 095fbd7 on master.

[mfateev]

I would make this package non Cadence specific. So this method would be something like:

func (c *Collection) GetIntPropertyWithStringFilter(key Key, defaultValue int, filter Filter) func(string) int {
	return func(filterValue string) int {
 		filters := make(map[Filter]interface{})
 		filters[filter] = filterValue
 		val, err := c.client.GetValueWithFilters(key, filters)
 		if err != nil {
 			return defaultVal
 		}
 		return val.(int)
 	}
 }

Then the code that uses it would make it task list specific. If the task list appears as filter in a lot of places the
then the following function (outside of this package) can be created:

func GetIntPropertyWithTaskListFilter(c *Collection, key Key, defaultValue int) func(string) int {
      return c.GetIntPropertyWithString(key, defaultValue, TaskListName)
}

[madhuravi]

What's the benefit of a public generic function? Makes sense as a private method if we have public methods for both task list and domain name.
Also all these methods are for Cadence, what is a non-Cadence use case for this?

[mfateev]

Each of Get methods repeats the same code snippet. What about:

func (c *Collection) GetProperty(key Key, defaultVal interface{}) func() interface{} {
       return func() int {
 		val, err := c.client.GetValue(key)
 		if err != nil {
 			return defaultVal
 		}
 		return val
 	}
}

Then Get methods become like:

func (c *Collection) GetFloat64Property(key Key, defaultVal float64) func() float64 {
      return func() float64 { 
          return GetProperty(key, defaultVal)().(float64) 
      }
}

[madhuravi]

Good idea.

[madhuravi]

With generic functions here and the previous filter one as well, there is an extra cast for converting defaultValue from empty interface to the corresponding type. I wrote some benchmark tests to compare the two and the previous non-generic approach is about 20% faster (260 versus 210 ns). I would favor performance here over reducing duplication since this function could be called quite frequently based on where it's being used. Let me know what you think. Will push my commit with benchmark test so you can take a look.

[madhuravi]

go test ./common/service/dynamicconfig/ -bench=.
goos: darwin
goarch: amd64
pkg: github.com/uber/cadence/common/service/dynamicconfig
BenchmarkGetIntProperty-8 5000000 279 ns/op
BenchmarkGetIntPropertyOld-8 10000000 219 ns/op
PASS
ok github.com/uber/cadence/common/service/dynamicconfig 4.122s

[mfateev]

I would break this into two packages dynamicconfig and cadenceconfig. The content of this file doesn't belong to generic dynamicconfig one.

[madhuravi]

Same as above, not sure what the non-cadence usage is. The dynamic config is solely for cadence.

[mfateev]

May be introduce small helper method?
s.matchingEngine.config.LongPollExpirationInterval = DurationConfigMillis(10)

[mfateev]

rename defaultVal to defaultValue in all methods.

[madhuravi]

sure but why? Go usually advocates for short variable names as opposed to Java eg. https://talks.golang.org/2014/names.slide#6

[yiminc-zz]

This will create an array variable every time this method is called.
I think a simple switch case clause is easier to understand and more performant.
switch k {
case unknownKey:
return "unknownKey"
case MinTaskThrottlingBurstSize:
return "minTaskThrottlingBurstSize"
...
}

[madhuravi]

Moved it out so array variable is not created everytime.

[samarabbas]

Everywhere config is read now needs to know what kind of filters need to be passed in. Can we just have a config object per entity type so all that semantics can be in a single place?
Basically use the exact same config mechanism and have a closure which remembers the name of tasklist when the closure was created. Then the above code will look like:

tasklistMgrConfig.MinTaskThrottlingBurstSize()

[madhuravi]

Done.

Addressed comments.