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

Add dynamic config change diff logging #2494

Merged
merged 7 commits into from
Feb 23, 2022

Conversation

Ardagan
Copy link
Contributor

@Ardagan Ardagan commented Feb 11, 2022

What changed?
Changed diff detection on dynamic config.

Why?
Fix incorrect behavior.

How did you test it?
local/unit testing

Potential risks
Dynamic config is not updated correctly.

Is hotfix candidate?
no

@Ardagan Ardagan force-pushed the DynamicConfigLogging branch from 4851ea6 to 11b62f6 Compare February 11, 2022 19:43
common/dynamicconfig/file_based_client.go Outdated Show resolved Hide resolved
common/dynamicconfig/file_based_client.go Show resolved Hide resolved
common/dynamicconfig/interface.go Outdated Show resolved Hide resolved
common/dynamicconfig/constants.go Show resolved Hide resolved
common/dynamicconfig/shared_constants.go Show resolved Hide resolved
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

Sorry, I probably lack some context here. Have we considered changing the logging behavior at the dynamic config collection level instead of the client? At client level, we will need the logging logic for each client implementation.

common/dynamicconfig/file_based_client.go Outdated Show resolved Hide resolved
common/dynamicconfig/file_based_client.go Outdated Show resolved Hide resolved
@Ardagan Ardagan force-pushed the DynamicConfigLogging branch from 11b62f6 to b52f823 Compare February 23, 2022 19:45
@Ardagan Ardagan marked this pull request as ready for review February 23, 2022 20:29
@Ardagan Ardagan requested a review from a team as a code owner February 23, 2022 20:29
@yiminc yiminc merged commit 4c212e5 into temporalio:master Feb 23, 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