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

Feature/zonal isolation zone discovery #6301

Conversation

davidporter-id-au
Copy link
Contributor

@davidporter-id-au davidporter-id-au commented Sep 23, 2024

What changed?

This changes the config property system.AllIsolationGroups from being a fixed static configuration value loaded on startup to be a callback which can be updated more dynamically. This should continue working with the existing dynamic config property as before, but support passing in a more complex discovery mechanism for isolation groups as needed.

Why?

We found that we were having trouble updating this config in production and needed to refactor it slightly to allow for a better update system.

How did you test it?

Deployed in staging environments with some initial manual testing as well as the unit testing.

Potential risks

This could break zonal isolation-the feature if it is wrong or buggy. that feature is designed to fall back to tasks being simply un-isolated, so it's not expected to actually break task processing, but it could degrade it if it were to contain some unforseen problems.

Release notes

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 73.68421% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.57%. Comparing base (3cacfbb) to head (9e3cf0e).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
common/resource/resourceImpl.go 40.00% 2 Missing and 1 partial ⚠️
service/matching/tasklist/task_list_manager.go 33.33% 2 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
...isolationgroup/defaultisolationgroupstate/state.go 87.15% <100.00%> (+15.72%) ⬆️
common/isolationgroup/isolationgroupapi/mappers.go 75.00% <100.00%> (+20.00%) ⬆️
service/matching/config/config.go 100.00% <100.00%> (ø)
service/matching/tasklist/task_list_manager.go 68.70% <33.33%> (ø)
common/resource/resourceImpl.go 2.88% <40.00%> (+0.60%) ⬆️

... and 33 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cacfbb...9e3cf0e. Read the comment docs.

@@ -124,3 +127,15 @@ func Test_anyToString(t *testing.T) {
res := anyToString(info, false, 100)
assert.Equal(t, "{Name:Joel, Number:1234, Time:2019-01-15 14:30:45 +0000 UTC}", res)
}

func TestJSONHistorySerializer_Serialize(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just added because I needed more test coverage

@davidporter-id-au davidporter-id-au merged commit f8d70de into cadence-workflow:master Oct 3, 2024
19 of 20 checks passed
Shaddoll added a commit to Shaddoll/cadence that referenced this pull request Oct 15, 2024
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.

2 participants