cleanup: don't return an internal type from VolumeGroupJournal.Connect() #4491
Conversation
| @@ -115,14 +115,14 @@ func (sgj *volumeGroupJournalConfig) Connect( | |||
| monitors, | |||
| namespace string, | |||
| cr *util.Credentials, | |||
| ) (*volumeGroupJournalConfig, error) { | |||
There was a problem hiding this comment.
we might get into problem when we serve many requests for different pools because all the journaling interface are initialized once and when are using different pool or ceph users we get connection for each one and that's the reason for returning which helps in initializing the connections, we cannot call Connect for same global object from multiple places. for now lets keep this and adjust it later or remove connect from the interface and modify it to take the input struct and return interface from it.
There was a problem hiding this comment.
Currently it returns a pointer to itself, not a new instance of volumeGroupJournalConfig.
Why would someone not create multiple VolumeGroupJournal instances for different pools? If the connection/credentials to the Ceph cluster are the same, they will share the ClusterConnection already.
There was a problem hiding this comment.
probably we might need to address it when required, For now approving it as there is no consumer of this yet
| @@ -115,14 +115,14 @@ func (sgj *volumeGroupJournalConfig) Connect( | |||
| monitors, | |||
| namespace string, | |||
| cr *util.Credentials, | |||
| ) (*volumeGroupJournalConfig, error) { | |||
There was a problem hiding this comment.
probably we might need to address it when required, For now approving it as there is no consumer of this yet
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 32de268 |
The VolumeGroupJournal interface does not need to return anything except for a potential error. Any instance that implements the VolumeGroupJournal interface can be used to call all functions. Signed-off-by: Niels de Vos <ndevos@ibm.com>
nixpanic
force-pushed
the
cleanup/VolumeGroupJournal.Connect
branch
from
06f098e
to
4f9ece6
Compare
|
/test ci/centos/k8s-e2e-external-storage/1.27 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.27 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/k8s-e2e-external-storage/1.29 |
|
/test ci/centos/mini-e2e/k8s-1.27 |
|
/test ci/centos/mini-e2e-helm/k8s-1.29 |
|
/test ci/centos/k8s-e2e-external-storage/1.28 |
|
/test ci/centos/mini-e2e/k8s-1.29 |
|
/test ci/centos/mini-e2e-helm/k8s-1.28 |
|
/test ci/centos/mini-e2e/k8s-1.28 |
The VolumeGroupJournal interface does not need to return anything except for a potential error. Any instance that implements the VolumeGroupJournal interface can be used to call all functions.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)