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

Refactor dynamic config #4863

Merged
merged 2 commits into from
Jun 29, 2022
Merged

Conversation

Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Jun 10, 2022

What changed?

  • Refactored dynamic config to enforce type of dynamic properties
  • Refactored dynamic config to provide a source of truth for default values of dynamic properties

Why?
The default values and types of dynamic properties are declared in comments and therefore are not enforced in our code. People can make a mistake when using these dynamic properties.

How did you test it?
unit test

Potential risks
the default value of system.enableVisibilitySampling was incorrectly set to true in frontend, and we fixed it in this PR.
If the frontend was depending on the default value of this property, the behavior is changed in this PR, which means read from elasticsearch is not sampled.

Release notes

Documentation Changes

@Shaddoll Shaddoll force-pushed the dynamic-refactor branch 5 times, most recently from a82f628 to a5beda0 Compare June 16, 2022 19:43
@Shaddoll Shaddoll force-pushed the dynamic-refactor branch 7 times, most recently from cb38edc to 088bf57 Compare June 25, 2022 01:06
@coveralls
Copy link

coveralls commented Jun 25, 2022

Pull Request Test Coverage Report for Build 0181b111-9edb-4833-a669-2eceade3c3c4

  • 643 of 801 (80.27%) changed or added relevant lines in 28 files are covered.
  • 113 unchanged lines in 15 files lost coverage.
  • Overall coverage increased (+0.002%) to 57.961%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/worker/scanner/tasklist/scavenger.go 1 2 50.0%
tools/cli/adminConfigStoreCommands.go 0 1 0.0%
cmd/server/cadence/server.go 0 2 0.0%
service/worker/scanner/executions/concrete_execution.go 7 9 77.78%
service/worker/scanner/executions/current_execution.go 7 9 77.78%
service/worker/scanner/timers/timers.go 7 9 77.78%
service/frontend/adminHandler.go 7 10 70.0%
tools/cli/domainUtils.go 0 3 0.0%
common/dynamicconfig/file_based_client.go 23 27 85.19%
host/dynamicconfig.go 12 16 75.0%
Files with Coverage Reduction New Missed Lines %
common/dynamicconfig/inMemoryClient.go 1 83.53%
common/dynamicconfig/nopClient.go 1 58.54%
host/dynamicconfig.go 1 68.69%
common/persistence/sql/workflowStateNonMaps.go 2 87.88%
common/dynamicconfig/config.go 3 86.63%
service/history/task/task.go 3 77.06%
service/history/task/transfer_standby_task_executor.go 3 89.32%
common/dynamicconfig/configstore/config_store_client.go 4 72.65%
service/history/decision/handler.go 4 72.21%
service/history/task/redispatcher.go 4 89.67%
Totals Coverage Status
Change from base Build 0181ae4f-d8b4-4177-8074-afb189b435d3: 0.002%
Covered Lines: 84026
Relevant Lines: 144970

💛 - Coveralls

@Shaddoll Shaddoll changed the title [WIP]Dynamic refactor Refactor dynamic config Jun 27, 2022
@Shaddoll Shaddoll marked this pull request as ready for review June 27, 2022 20:52
tools/cli/domainUtils.go Outdated Show resolved Hide resolved
@Shaddoll Shaddoll force-pushed the dynamic-refactor branch 12 times, most recently from ab71d58 to 597b390 Compare June 28, 2022 23:45
@Shaddoll Shaddoll force-pushed the dynamic-refactor branch 2 times, most recently from 3c56a6c to bc10296 Compare June 29, 2022 17:45
Copy link
Member

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

Overall much improved I think, thanks a lot for doing this!

common/dynamicconfig/config.go Show resolved Hide resolved
@Shaddoll Shaddoll merged commit 650cf8a into cadence-workflow:master Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants