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.

Sign up for GitHub

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

Jump to bottom

added worker.NewV2 with validation on decision poller count #1370

Merged
merged 7 commits into from
Oct 10, 2024
Merged

added worker.NewV2 with validation on decision poller count #1370

merged 7 commits into from
Oct 10, 2024

Conversation

shijiesheng
Copy link
Member

@shijiesheng shijiesheng commented Oct 8, 2024 •
edited
Loading

What changed?

  • added a worker.NewV2 interface that will return error and a deprecation message on New api.
  • added validation on WorkerOptions
    -- decision poller should be larger than 1 if value is set (not zero) and sticky execution is enabled.

Why?
#1369

How did you test it?
Unit Test

Potential risks
Low

===== below is required for breaking change ======

Detailed Description
added a new worker.NewV2 interface that will return error and a deprecation message on New api.

Impact Analysis

  • Backward Compatibility: yes
  • Forward Compatibility: yes

Testing Plan

  • Unit Tests: yes
  • Persistence Tests: not related
  • Integration Tests: not related
  • Compatibility Tests: tested in Uber internal canary test

Rollout Plan

  • What is the rollout plan? version upgrade and change usage of New to NewV2
  • Does the order of deployment matter? no
  • Is it safe to rollback? Does the order of rollback matter? yes
  • Is there a kill switch to mitigate the impact immediately? No

@codecov Codecov
Copy link

codecov bot commented Oct 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.46%. Comparing base (4d4c09f) to head (9d1627c).
Report is 1 commits behind head on master.

Additional details and impacted files
Files with missing lines Coverage Δ
internal/internal_poller_autoscaler.go 91.95% <ø> (ø)
internal/internal_worker.go 80.57% <100.00%> (+0.08%) ⬆️
internal/worker.go 33.33% <100.00%> (+19.04%) ⬆️

... and 2 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 4d4c09f...9d1627c. Read the comment docs.

Groxx
Groxx reviewed Oct 9, 2024
View reviewed changes
internal/worker.go Outdated Show resolved Hide resolved
Groxx
Groxx reviewed Oct 9, 2024
View reviewed changes
worker/worker.go Show resolved Hide resolved
@shijiesheng shijiesheng changed the title enforce having larger than one decision poller for sticky execution e… added worker.NewV2 with validation on decision poller count Oct 9, 2024
taylanisikdemir
taylanisikdemir reviewed Oct 10, 2024
View reviewed changes
worker/worker.go Outdated Show resolved Hide resolved
worker/worker.go Outdated
@@ -277,12 +277,36 @@ const (
// identifies group of workflow and activity implementations that are
// hosted by a single worker process
// options - configure any worker specific options like logger, metrics, identity
//
// DEPRCATED: use NewV2 instead since this implementation will panic on error
Copy link
Member

Choose a reason for hiding this comment

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

typo and also change casing to // Deprecated as @Groxx mentioned in other comment

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

worker/worker.go
@@ -277,12 +277,36 @@ const (
// identifies group of workflow and activity implementations that are
// hosted by a single worker process
// options - configure any worker specific options like logger, metrics, identity
//
// DEPRCATED: use NewV2 instead since this implementation will panic on error
func New(
Copy link
Member

Choose a reason for hiding this comment

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

please update CHANGELOG.md about this new validation and the new worker.NewV2 constructor

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

Groxx
Groxx approved these changes Oct 10, 2024
View reviewed changes
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.

thanks!

@shijiesheng shijiesheng merged commit 89676d3 into cadence-workflow:master Oct 10, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylanisikdemir taylanisikdemir taylanisikdemir left review comments

@abhishekj720 abhishekj720 abhishekj720 approved these changes

@Groxx Groxx Groxx approved these changes

@jakobht jakobht Awaiting requested review from jakobht jakobht is a code owner

@3vilhamster 3vilhamster Awaiting requested review from 3vilhamster 3vilhamster is a code owner

@dkrotx dkrotx Awaiting requested review from dkrotx dkrotx is a code owner

@demirkayaender demirkayaender Awaiting requested review from demirkayaender demirkayaender is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shijiesheng @Groxx @taylanisikdemir @abhishekj720