K8SPS-266: Refactor Replicator interface #517
Conversation
inelpandzic
requested review from
tplavcic,
nmarukovich and
ptankov
as code owners
| @@ -0,0 +1,240 @@ | |||
| package db | |||
There was a problem hiding this comment.
I’m not sure about this cmd/db. For me cmd should be consumers of the API not providers and I find depending on a cmd package from another cmd package a bit counterintuitive.
what do you think of something like this?
pkg/mysql
├── client
│ ├── direct_client.go
│ └── exec_client.go
├── config.go
├── mysql.go
└── topology
└── topology.go
and consumers can create instances like:
client.NewExecClient(...)
client.NewDirectClient(...)
There was a problem hiding this comment.
cmd/mysql/client.go `ReplicationManager`
- StartReplication(ctx context.Context, host, replicaPass string, port int32) error
- StopReplication(ctx context.Context) error
- ResetReplication(ctx context.Context) error
- ReplicationStatus(ctx context.Context) (db.ReplicationStatus, string, error)
- IsReplica(ctx context.Context) (bool, error)
- DisableSuperReadonly(ctx context.Context) error
- IsReadonly(ctx context.Context) (bool, error)
- ReportHost(ctx context.Context) (string, error)
- CloneInProgress(ctx context.Context) (bool, error)
- Clone(ctx context.Context, donor, user, pass string, port int32) error
- DumbQuery(ctx context.Context)
- GetMemberState(ctx context.Context, host string) (db.MemberState, error)
- CheckIfInPrimaryPartition(ctx context.Context) (bool, error)
- EnableSuperReadonly(ctx context.Context) error
- Close() error
pkg/mysql/client/replication.go `ReplicationManager`
- ChangeReplicationSource(ctx context.Context, host, replicaPass string, port int32) error
- ReplicationStatus(ctx context.Context) (ReplicationStatus, string, error)
- GetGroupReplicationPrimary(ctx context.Context) (string, error)
- GetGroupReplicationReplicas(ctx context.Context) ([]string, error)
- GetMemberState(ctx context.Context, host string) (MemberState, error)
pkg/mysql/client/users.go `UserManager`
- DiscardOldPasswords(ctx context.Context, users []mysql.User) error
- UpdateUserPasswords(ctx context.Context, users []mysql.User) error
As you can see the structure of these three components, there are three different and distinct things, meaning, there is nothing in common between them so we could have a single interface with different implementations. If we want some interfaces they would be like three different ones with implementation for each of them and that is not necessary. If we want something that you suggested, we would more-or-less have something similar that we had before, and I wouldn't like that.
Thats why I still suggest a structure like this, with only types that expose certain methods for these three use cases. The thing that I think is good from the suggestion is the location of everything. I like more to have pkg/mysql/client/... rather than pkg/db, much better.
And regarding your concern Ege about like a cmd importing cmd/db, that is OK and it should not necesseraliy come from pkg/..., because it is related only to cmd. Imagine having some common util that is needed for different stuff in cmd, you wouldn't put it in pkg. And because I also wanted to confirm this opinion of mine, I looked at Docker and Kubernetes source code and this is a common thing there as well.
With all of this, everything in here is nicely separated and clear, and most importantly, simple without unnecessary complications.
| Replicas []string | ||
| } | ||
|
|
||
| func (r *PerconaServerMySQLBackupReconciler) getTopology(ctx context.Context, cluster *apiv1alpha1.PerconaServerMySQL, operatorPass string) (Topology, error) { |
There was a problem hiding this comment.
I don't like the idea of removing the pkg/mysql/topology package and adding a new method to the reconciler. This could potentially make the reconciler excessively complex and difficult to refactor when making changes related to topology, such as the ones proposed in this PR.
I think the reconciler should have methods that are only responsible for managing the cluster, without retrieving any data from the database. The pkg/db package is more suitable for that purpose.
If the topology is only used for the pkg/controller/psbackup package, then let's move pkg/mysql/topology to the pkg/controller/psbackup/topology
There was a problem hiding this comment.
I see your point, but why I don't necessarily agree is that of having a whole package for only one thing used in one place. And the reconciler is calling the database all over the place, the way it manages the cluster involves calling DB, so I don't see the problem with this in that sense. I can agree that having a method on the reconciler like this is not nice, but having a simple function is OK in my opinion.
I figured out that you introduced this topology package because of the proposed PR, but what I suggest is that we don't design our code for the future that might never happen, it can just complicate things uneccesarily. When we need it we can introduce whatever we want.
There was a problem hiding this comment.
I agree that it's too much to create a separate package. But I don't want to add another method to reconciler, even if it's simple. We can try to make it a function instead of a method. We can also just put this function in a separate file, such as pkg/mysql/psbackup/topology.go. What do you think?
Actually, I don't think my suggestion is worth wasting time on. This is not a critical problem, so I'll approve the PR as soon as @egegunes' suggestions are resolved.
There was a problem hiding this comment.
I agree with you Andrii, getting the topology from the reconciler is not that nice, like I said, better maybe to have it in a function. But even if we have it it will not stick out, we have something similar with isGRReady on PerconaServerMySQLReconciler.
commit: b184ccf |
CHANGE DESCRIPTION
Problem:
The
Replicatorinterface was very bloated and could be refactored to be much simpler.Solution:
Remove the
Replicatorinterface altogether by essentially splitting it into two parts - the one used in our binariescmd/healthcheckandcmd/bootstrap(connects to mysql via the driver - directly) and other that is used by the operator (connects to mysql viaexec).Now we have:
cmd/db DBtype which is used byhealthcheckandbootstrappkg/db dbtype which is used by the operatorpkg/db dbis encapsulated inpkg/db ReplicationDBManagerused inps/controllerandpsbackup/controller, and inpgk/db UserDBManagerused inps/user.Besides this two more things are refactored:
execin the name that was left when making the operator run localyThis PR fixes this task as well: https://perconadev.atlassian.net/browse/K8SPS-256
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability