Separate connection pools for reader and writer. #1451
Conversation
063aca7 to
3c2cc76
Compare
c7ccaed to
4900a65
Compare
| // NewClient creates a new PostgreSQL client | ||
| func NewClient(cfg *Config, mt tenancy.Authorizer, schemaLocker LockFunc, readOnly bool) (*Client, error) { | ||
| pgConfig, numCopiers, err := getPgConfig(cfg) | ||
| func NewClient(r prometheus.Registerer, cfg *Config, mt tenancy.Authorizer, schemaLocker LockFunc, readOnly bool) (*Client, error) { |
There was a problem hiding this comment.
Would it make sense to refactor this into 2 different functions? Something like
- NewClient
- NewReadOnlyClient
And moving the code that's inside the !readOnly into a getWriterPool function and the other part into a getReaderPool function then the Client constructors will just call this functions and pass the values to NewClientWithPool
There was a problem hiding this comment.
This is a good idea. But, given the current PR, it will be out of scope plus a lot of changes plus the existing stuff in this PR. Would you like to work on this refactoring once we merge this PR?
There was a problem hiding this comment.
Let's create an issue for the refactoring. @alejandrodnm would you mind making it since it is your proposal? 😄
There was a problem hiding this comment.
I knew it was a trap 😄 Yeah, I'll do it
4900a65 to
0d1203d
Compare
56185bb to
1127880
Compare
| if client == nil { | ||
| continue | ||
| } | ||
| pgDelete := deletePkg.PgDelete{Conn: client.Connection} |
There was a problem hiding this comment.
Should we use writerPool for series deletion?
There was a problem hiding this comment.
While that is right, I think for now, lets keep deletion from read connection. Deletion is non-frequent, but when executed, it blocks a connection for sometime. So, its good that we don't consume the writer pool connection for this atm.
antekresic
left a comment
There was a problem hiding this comment.
I'm not sure removing a configuration flag like max conns is what we want or that it was decided like that in the design doc.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit separates the reader and writer connection pools. By default, reader gets 0.3 and writer gets 0.5 times of the allowed connection from the db. But, in case of readonly mode, reader gets 0.5 times the connections from the db.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
1127880 to
1a853c7
Compare
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
1a853c7 to
da4b651
Compare
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
4a43643 to
77a69b1
Compare
| return 0, fmt.Errorf("%s pool size canot be greater than the 'max_connections' allowed by the database", poolName) | ||
| default: | ||
| } | ||
| if cfg.UsesHA { |
There was a problem hiding this comment.
Nit: doing this check two times and you can do it just once. Improve if you can but lets not waste time on it.
ef0b54c to
e29d33b
Compare
e29d33b to
0465c0e
Compare
Signed-off-by: Harkishen-Singh harkishensingh@hotmail.com
This commit separates the reader and writer connection pools.
By default, reader gets 0.3 and writer gets 0.5 times of
the allowed connection from the db. But, in case of readonly mode,
reader gets 0.5 times the connections from the db.
TODO
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: