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

Routing logic to take both NamespaceID and workflowID into account #629

Merged
merged 10 commits into from
Jul 31, 2020

Conversation

samarabbas
Copy link
Contributor

What changed?
Update the routing logic to use both NamespaceID and workflowID to
compute the hash used for routing logic.

Why?
Cadence only uses workflowID as the key to compute the hash which is used for routing requests to
shard hosting the workflow execution. This is a problem as multiple namespaces could use the same
workflowID and this logic could result in hot partition problems.
Updated the routing logic to use both namespaceID and workflowID to compute the hash key.

How did you test it?
unit and integration tests

Potential risks
This is a non backwards compatible change so this is why we are making it before our first production release.

Update the routing logic to use both NamespaceID and workflowID to
compute the hash used for routing logic.
@@ -237,7 +237,10 @@ func (p *esProcessorImpl) hashFn(key interface{}) uint32 {
return 0
}
numOfShards := p.config.IndexerConcurrency()
return uint32(common.WorkflowIDToHistoryShard(id, numOfShards))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no need to use WorkflowIDToHistoryShard as the hashing function. All this logic needed is a hashing function which takes in a string and computes range hashKey which is used to route the call to worker go routine processing ES messages.

@@ -403,7 +403,8 @@ message DescribeHistoryHostRequest {
//ip:port
string host_address = 1;
int32 shard_id_for_host = 2;
temporal.api.common.v1.WorkflowExecution execution_for_host = 3;
string namespace_id = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Why it is namespace above and namespace_id here?

Copy link
Member

Choose a reason for hiding this comment

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

I guess for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They represent 2 different identifiers for namespace. namespace is used whenever API takes namespace name and namespace_id is used whenever API takes internal uuid which represents the namespace. Internally within the system we use namespace_id as the identifier.

hash := farm.Fingerprint32([]byte(workflowID))
// WorkflowIDToHistoryShard is used to map namespaceID-workflowID pair to a shardID
func WorkflowIDToHistoryShard(namespaceID, workflowID string, numberOfShards int) int {
idBytes := []byte(namespaceID + "_" + workflowID)
Copy link
Member

Choose a reason for hiding this comment

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

What does separator provides? We are hashing it right away.

@samarabbas samarabbas merged commit e299cdd into temporalio:master Jul 31, 2020
@samarabbas samarabbas deleted the workflow-request-routing branch July 31, 2020 01:22
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