[WIP][history] refactor history client with redirect wrapper #5762
Conversation
shijiesheng
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
shijiesheng
force-pushed
the
history-client-redirect
branch
from
0b569e2
to
76b624f
Compare
Codecov Report
Additional details and impacted files
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -950,6 +954,11 @@ func (v *HistoryStartWorkflowExecutionRequest) GetPartitionConfig() (o map[strin | |||
| return | |||
| } | |||
|
|
|||
| // ToRedirectKey convert request to redirect key | |||
| func (v *HistoryStartWorkflowExecutionRequest) ToRedirectKey() RedirectKey { | |||
There was a problem hiding this comment.
unit test please :)
| @@ -100,3 +102,20 @@ func (pr PeerResolver) GetAllPeers() ([]string, error) { | |||
| } | |||
| return peers, nil | |||
| } | |||
|
|
|||
| // FromRedirectKey resolve the peer from types.RedirectKey by a specific order | |||
| func (pr PeerResolver) FromRedirectKey(key types.RedirectKey) (string, error) { | |||
There was a problem hiding this comment.
I guess the other PeerResolver.From* functions on can be made private once all usage is refactored. Is that the plan?
There was a problem hiding this comment.
Yes that's a good suggestion
| @@ -22,6 +22,7 @@ package history | |||
|
|
|||
| import ( | |||
| "context" | |||
| "github.com/uber/cadence/common/peerresolver" | |||
| if key.HostAddress != "" { | ||
| return pr.FromHostAddress(key.HostAddress) | ||
| } | ||
| return "", fmt.Errorf("RedirectKey is not valid: %v", key) |
There was a problem hiding this comment.
let's add unit tests
| @@ -100,3 +102,20 @@ func (pr PeerResolver) GetAllPeers() ([]string, error) { | |||
| } | |||
| return peers, nil | |||
| } | |||
|
|
|||
| // FromRedirectKey resolve the peer from types.RedirectKey by a specific order | |||
There was a problem hiding this comment.
let's document the "specific order" so it's more explicit in documentation
|
no other use cases for the wrapper and might just leave it as it is. Also tests are already covering this. |
What changed?
FromRedirectKeymethodWhy?
simplify history client to make testing easier
How did you test it?
Potential risks
Release notes
Documentation Changes