Refactor connection pooler #1159

RafiaSabih · 2020-10-05T10:50:40Z

Refactor connection pooler to be a map of PostgresRole, as now we require connection pooler for each service.

FxKu · 2020-10-07T08:21:24Z

@RafiaSabih can you rebase your PR, so that it does not list unrelated commits?

- Refactor code for connection pooler deployment and services - Refactor sync code for connection pooler - Rename EnableConnectionPooler to EnableMasterConnectionPooler - Update yamls and tests

- Update deleteConnectionPooler to include role - Rename EnableMasterConnectionPooler back to original name for backward compatiility - other minor chnages and code improvements

- Refactor needConnectionPooler for master and replica separately - Improve sync function - Add test cases to create, delete and sync with repplica connection pooler Other changes

- Fixed the issue with failing test cases - Add more test cases for replica connection pooler - Added docs about the new flag

Have one unified function to tell if any connection pooler is required Add a helper function to list the roles that require connection pooler, helps in avoiding duplication of code

…erator into pooler-refac

RafiaSabih · 2020-10-08T15:02:05Z

Current issues open for discussion,

some of the functions in connection_pooler package still require cluster package for things like Spec (to get the values of flag EnableConnectionPooler, etc.), OpConfig( to get ConnectionPooler for schema, user, etc. and also for some not so related stuff like PodServiceAccountName).
For connection pooler related stuff, maybe we can have separate config for it and thus would not require cluster package for them and also they are not used anywhere else. For PodServiceAccountName, I am not very sure how to handle that.
Other issue is in accessing c.KubeClient, which is also not very clear to me on how to handle that dependency

One way could be to let the functions like createConnectionPooler, deleteConnectionPooler, updateConenctionPooler which are using Kubeclient in the cluster package itself, but then it kind of defeats the purpose of refactoring.
Another way could be to have a third package which does this stuff for connection pooler and maybe other resources and both cluster and connection pooler could use import it.

sdudoladov · 2020-10-13T05:12:19Z

pkg/cluster/cluster.go

 	processMu        sync.RWMutex // protects the current operation for reporting, no need to hold the master mutex
 	specMu           sync.RWMutex // protects the spec for reporting, no need to hold the master mutex
-
+	ConnectionPooler map[PostgresRole]*connection_pooler.ConnectionPoolerObjects


So this whole PR is not solely a refactoring but rather a code change that enables separate connection poolers for the master and replicas ?

sdudoladov · 2020-10-13T05:13:43Z

pkg/cluster/cluster.go

+
+		roles := c.RolesConnectionPooler()
+		for _, r := range roles {
+			c.logger.Warningf("found roles are %v", r)


why Warningf and not Infof ? at this point there is technically nothing bad happening

sdudoladov · 2020-10-13T05:16:12Z

pkg/cluster/cluster.go

+
+		for _, r := range c.RolesConnectionPooler() {
+			if c.ConnectionPooler[r] != nil {
+				c.logger.Warning("Connection pooler already exists in the cluster")


It would be more descriptive here to log also the role for which the pooler exists, and the name of this pooler similar to how errors are processed below in this code

sdudoladov · 2020-10-13T05:22:02Z

pkg/cluster/cluster.go

 	return nil
 }

+func syncResources(a, b *v1.ResourceRequirements) bool {


I understand this function was named like that in the original code, but judging from what it does, shouldSyncResources or needToSyncResouces is a better name for it from a readability perspective.

sdudoladov · 2020-10-13T05:24:37Z

pkg/pooler_interface/pooler_interface.go

+package pooler_interface
+
+//functions of cluster package used in connection_pooler package
+type pooler interface {


Is this interface meant to help resolving cyclic dependencies ?

sdudoladov · 2020-10-13T05:26:52Z

pkg/connection_pooler/connection_pooler.go

+	pgPort                    = 5432
+)
+
+// K8S objects that are belongs to a connection pooler


typo: are belongs -> belong

sdudoladov · 2020-10-13T05:29:16Z

pkg/connection_pooler/connection_pooler.go

+
+const (
+	// Master role
+	Master PostgresRole = "master"


Role constants are already defined in pkg/cluster/types.go. Why are they re-defined here ?

sdudoladov · 2020-10-13T06:17:00Z

Current issues open for discussion,

I feel they are all caused by the fundamental circular dependency between pkg/cluster and pkg/connection_pooler

We can try to resolve the dependency by making the pkg/connection_pooler depend on an additional ClusterInterface instead of directly importing objects from pkg/cluster. The interface would

live in the connection_pooler package
contain all methods required by a connection pooler

One can have the effective implementation of the interface in the cluster package. When the operator creates a PG cluster, it will just provide an implementation of the ClusterInterface to a connection pooler.

sdudoladov · 2020-10-13T06:38:03Z

@RafiaSabih could you update the PR description with

the scope of this PR. It does not seem to me to be limited to refactoring only as it also manipulates with the objects belonging to the replica connection pooler
the relationship between this PR and #1127 . Do you still work on the latter ? there are commits there from the last week

Kepp connectionPooler in cluster package itself. This helps in getting rid of previously added packages and interfaces. Based on the new structure of ConnectionPooler in cluster, there are functional changes to come. This commit is only for updating and cleaning of packages.

create and sync connectionPooler were doing essentially the same task with respect to creating new connectionPooler if not present. Hence, the code from create is already present in sync, which is now utilized and duplicate code is removed. Other code duplication is also removed in sync, for deciding upon when to delete the connectionPooler. Basically, anytime newNeedConnectionPooler is false, we have to delete it, if present. Other respective modifications in tests.

RafiaSabih · 2020-10-29T07:43:17Z

👍

Jan-M · 2020-10-29T09:17:38Z

👍

erthalion · 2020-10-29T10:51:36Z

pkg/cluster/connection_pooler.go

+
+//delete connection pooler
+func (c *Cluster) deleteConnectionPooler(role PostgresRole) (err error) {
+	//c.setProcessName("deleting connection pooler")


Should it be commented out?

erthalion · 2020-10-29T10:59:15Z

👍

First commit

c663b87

RafiaSabih requested review from avaczi, CyberDem0n, erthalion, FxKu, Jan-M and sdudoladov as code owners October 5, 2020 10:50

Rafia Sabih added 3 commits October 6, 2020 16:39

Make connection_pooler a separate package

d6b4f82

Add interface to use common functions

49f6285

place config in a common package

289b209

Rafia Sabih added 19 commits October 7, 2020 10:44

Add pooler for replica

403bbb6

Add new pooler service for replica

75ea994

Enable connection pooler for replica

c1e42a8

- Refactor code for connection pooler deployment and services - Refactor sync code for connection pooler - Rename EnableConnectionPooler to EnableMasterConnectionPooler - Update yamls and tests

Fix labels for the replica pods

dd18c18

fix for labels selector

4702340

Assorted changes

99abcdc

- Update deleteConnectionPooler to include role - Rename EnableMasterConnectionPooler back to original name for backward compatiility - other minor chnages and code improvements

Fix sync

0ce9b50

Adding test cases and other changes

6d4bb18

- Refactor needConnectionPooler for master and replica separately - Improve sync function - Add test cases to create, delete and sync with repplica connection pooler Other changes

Improvements in tests

6fda51c

- Fixed the issue with failing test cases - Add more test cases for replica connection pooler - Added docs about the new flag

Resolve review comments

a03397a

Minor fix

c692ca2

Refactor needConnectionPooler

175f0c5

Have one unified function to tell if any connection pooler is required Add a helper function to list the roles that require connection pooler, helps in avoiding duplication of code

Update tests

fea359f

Add sync test

6272377

Add and update tests

3ba73fb

Cleanup deleteConnectionPooler

bb51aba

Add labels

84fbfe3

First commit

c63446c

Make connection_pooler a separate package

ae3f5ef

Merge branch 'pooler-refac' of https://github.com/zalando/postgres-op…

11cd11f

…erator into pooler-refac

sdudoladov reviewed Oct 13, 2020

View reviewed changes

sdudoladov added this to the 1.6 milestone Oct 13, 2020

sdudoladov added zalando enhancement labels Oct 13, 2020

Rafia Sabih added 13 commits October 19, 2020 09:37

Update connection_pooler funcs accordingly

20f2fb7

fix test cases

60460c3

avoid passing spec unnecessarily

5828e2f

Fix sync/create issues

7112995

fix podSpec test

cbdc346

isolate tests and other updates

72e2af8

Refactor more methods

3d3bc14

minor fix in test

e88fa06

another test case fix

4354b02

Nitpicks and other changes

2ed0e05

cleanup and avoid using cluster in more functions

07bb270

Rafia Sabih added 2 commits October 29, 2020 09:17

resolve conflicts

4c70c55

fix

49613bd

erthalion reviewed Oct 29, 2020

View reviewed changes

minor fix

7b4e850

RafiaSabih merged commit 133f77c into replica-pooler Oct 29, 2020

RafiaSabih deleted the pooler-refac branch October 29, 2020 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor connection pooler #1159

Refactor connection pooler #1159

Uh oh!

RafiaSabih commented Oct 5, 2020

Uh oh!

FxKu commented Oct 7, 2020

Uh oh!

RafiaSabih commented Oct 8, 2020 •

edited by erthalion

Loading

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov Oct 13, 2020

Uh oh!

sdudoladov commented Oct 13, 2020 •

edited

Loading

Uh oh!

sdudoladov commented Oct 13, 2020

Uh oh!

RafiaSabih commented Oct 29, 2020

Uh oh!

Jan-M commented Oct 29, 2020

Uh oh!

erthalion Oct 29, 2020

Uh oh!

erthalion commented Oct 29, 2020

Uh oh!

Uh oh!

Refactor connection pooler #1159

Refactor connection pooler #1159

Uh oh!

Conversation

RafiaSabih commented Oct 5, 2020

Uh oh!

FxKu commented Oct 7, 2020

Uh oh!

RafiaSabih commented Oct 8, 2020 • edited by erthalion Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdudoladov commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdudoladov commented Oct 13, 2020

Uh oh!

RafiaSabih commented Oct 29, 2020

Uh oh!

Jan-M commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erthalion commented Oct 29, 2020

Uh oh!

Uh oh!

RafiaSabih commented Oct 8, 2020 •

edited by erthalion

Loading

sdudoladov commented Oct 13, 2020 •

edited

Loading