Create a single connection pool per host #359
Conversation
| @@ -128,7 +113,11 @@ func (s *TestBase) SetupWorkflowStoreWithOptions(options TestBaseOptions) { | |||
| if err != nil { | |||
| log.Fatal(err) | |||
| } | |||
| s.ExecutionMgrFactory = newTestExecutionMgrFactory(options, s.CassandraTestCluster, log) | |||
| s.ExecutionMgrFactory, err = NewCassandraPersistenceClientFactory(options.ClusterHost, options.ClusterPort, | |||
There was a problem hiding this comment.
is this creating real cassandra session for test?
There was a problem hiding this comment.
Yes we create a connection to cassandra for certain tests.
|
|
||
| // CreateExecutionManager implements ExecutionManagerFactory interface | ||
| func (f *cassandraPersistenceClientFactory) CreateExecutionManager(shardID int) (ExecutionManager, error) { | ||
| mgr, err := NewCassandraWorkflowExecutionPersistence(shardID, f.session, f.logger) |
There was a problem hiding this comment.
not sure about this, but is it ever possible that the session get into some bad state and needs to be destroyed and recreated?
There was a problem hiding this comment.
It is possible. But we have the similar issues for all other persistence client currently. Creating a separate client just for execution manager won't help much in such a situation.
Having a persistence client factory is the right abstraction to build this kind of resiliency in the future.
| history = persistence.NewHistoryPersistenceClient(history, base.GetMetricsClient()) | ||
| execMgrFactory := NewExecutionManagerFactory(&p.CassandraConfig, p.Logger, base.GetMetricsClient()) | ||
|
|
||
| execMgrFactory, err := persistence.NewCassandraPersistenceClientFactory(p.CassandraConfig.Hosts, |
There was a problem hiding this comment.
The mix match of terminology between "CassandraPersistence, CassandraHistoryPersistence, ExecutionManager and ExecutionManagerFactory" is turning out to be super confusing while navigating through the code. IMO, the layering between gocqlConnPool / persistence API / higher level APIs is also not very obvious because of this. Ideally, we should have one gocql client pool or factory that takes as input { keyspace, defaultConsistencyLevels, seeds } and vends a gocql.Session object ? And then on top of this, one single persistence API interface that lists all of our contracts with persistence. What do you think ?
Whatever we decide, this will likely be a separate work item
There was a problem hiding this comment.
I agree we need another pass to clean up the model. This change is just to have a single client per host for ExecutionManager which will be an improvement to what we have currently.
This is the reason I created PersistenceClientFactory, which should expose API for creating various persistence managers and also manage a pool of gocql clients.
| // to cassandra for this engine. So, make sure to | ||
| // close executionMgr only after stopping the engine | ||
| if i.executionMgr != nil { | ||
| i.executionMgr.Close() |
There was a problem hiding this comment.
Can you make the ExecutionManager not Closeable i.e. remove Close() from the interface as well ?
There was a problem hiding this comment.
I thought about that as it is little tricky right now. Once we move all the managers be created using the persistenceClientFactory then we can remove Close on all of them.
| @@ -62,6 +62,10 @@ type Config struct { | |||
| TransferProcessorUpdateAckInterval time.Duration | |||
| TransferProcessorForceUpdateInterval time.Duration | |||
| TransferTaskWorkerCount int | |||
|
|
|||
| // Persistence settings | |||
| ExecutionMgrNumConns int | |||
There was a problem hiding this comment.
any good reason these two should use a different connection pool ? They are both directing requests to the same keyspace, right ? Is the consistency level coupled with gocql.session today ?
There was a problem hiding this comment.
Agreed. I just created two knobs since we have two separate clients for these. We should clean it up when we can consolidate the gocql clients for all persistence interfaces.
| @@ -31,14 +31,18 @@ type Config struct { | |||
| DefaultVisibilityMaxPageSize int32 | |||
| DefaultHistoryMaxPageSize int32 | |||
| RPS int | |||
|
|
|||
| // Persistence settings | |||
| HistoryMgrNumConns int | |||
There was a problem hiding this comment.
same comments as above, around gocql connection pool layering
When history service comes up it creates a gocql client for execution manager and then uses the same client for all shards for making persistence calls. Also created config knobs to configure number of connections for both events and execution maanger clients.
fixes #307