Created Shard Manager Service #6297
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 63 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
jakobht
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
|
|
||
| s.Resource.Start() | ||
|
|
||
| <-s.stopC |
There was a problem hiding this comment.
What is this line for?
Awe we blocking on Start() call?
There was a problem hiding this comment.
Good question, I think it's a pretty questionable design, it's the same for all the services, e.g. matching (but also all the others):
https://github.com/uber/cadence/blob/6108a443a82b7998582fc8870e012a21ae0a353c/service/matching/service.go#L121
We cannot just remove it, this code depends on it blocking: https://github.com/uber/cadence/blob/fb1324877829f126af9d7e9760636240a1f92bcc/cmd/server/cadence/server.go#L338-L342
I don't know why we track the life cycle in two different places. I think the right way to fix is migrate to fx.
service/shardmanager/service_test.go
Outdated
| assert.Equal(t, int32(common.DaemonStatusStopped), atomic.LoadInt32(&service.status)) | ||
| } | ||
|
|
||
| func TestStartAndStopReturnsImmediatelyWhenAlreadyStopped(t *testing.T) { |
There was a problem hiding this comment.
nit: it is a bit misleading name ...ReturnsImmediatelyWhenAlreadyStopped because you're not really checking any side-effects.
There was a problem hiding this comment.
Renamed to TestStartAndStopDoesNotChangeStatusWhenAlreadyStopped
| go service.Start() | ||
|
|
||
| time.Sleep(100 * time.Millisecond) // The code assumes the service is started when calling Stop | ||
| assert.Equal(t, int32(common.DaemonStatusStarted), atomic.LoadInt32(&service.status)) |
There was a problem hiding this comment.
This Sleep requires to leverage Start() background activity.
Why Start() is not just blocking? Is that an approach we use for other services?
There was a problem hiding this comment.
See the comment for the first comment :-) - #6297 (comment)
- Renamed test
|
Just curious, is there more context available to read on this change? |
There is currently no publicly available context. The idea is to have a service that manages the shards in Cadence instead of relying on ringpop. |
Hi, getting this error on both local docker setup and k8s helm setup. Interim Resolution: Use ubercadence/server:v1.2.13-auto-setup |
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
This reverts commit 45d5403.
What changed?
Why?
This is the first step to creating a new shard manager service. This is the first step of the skeleton of the service, to build upon
How did you test it?
Unit tests and local runs
Potential risks
Release notes
Documentation Changes