-
Notifications
You must be signed in to change notification settings - Fork 917
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
Redirect replication requests when shard count is different #3789
Conversation
@@ -174,7 +170,7 @@ func (e *taskExecutorImpl) handleActivityTask( | |||
ctx, cancel := e.newTaskContext(ctx, attr.NamespaceId) | |||
defer cancel() | |||
|
|||
err = e.historyEngine.SyncActivity(ctx, request) | |||
_, err = e.shardContext.GetHistoryClient().SyncActivity(ctx, request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be additional cost when we always go through history client? Should we still go through history engine if the request is for the local shard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can certainly add additional wrapper, but the ROI is low?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, if we decide to do perf optimization, the optimization should be done within the wrapper of client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, if we decide to do perf optimization, the optimization should be done within the wrapper of client
Yes. There will be additional network cost but I don't think it is a concern at this point. I can also add an additional check for now and move it to the wrapper of client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Let's leave it for now and address it in case it becomes an issue in prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add a TODO there.
* Redirect replication requests when shard count is different
What changed?
Redirect replication requests when shard count is different
Why?
When replicate workflow between different shard count, it requires redirect to different shards.
How did you test it?
Unit tests
Potential risks
Is hotfix candidate?