Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: don't return an internal type from VolumeGroupJournal.Connect() #4491

Merged
merged 1 commit into from
Mar 14, 2024
Merged

cleanup: don't return an internal type from VolumeGroupJournal.Connect() #4491

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions internal/journal/volumegroupjournal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type VolumeGroupJournal interface {
Connect(
monitors,
namespace string,
cr *util.Credentials) (*volumeGroupJournalConfig, error)
cr *util.Credentials) error
// Destroy frees any resources and invalidates the journal connection.
Destroy()
// SetNamespace sets the namespace for the journal.
Expand Down Expand Up @@ -115,14 +115,14 @@ func (sgj *volumeGroupJournalConfig) Connect(
monitors,
namespace string,
cr *util.Credentials,
) (*volumeGroupJournalConfig, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we might need to address it when required, For now approving it as there is no consumer of this yet

) error {
conn, err := sgj.Config.Connect(monitors, namespace, cr)
if err != nil {
return nil, err
return err
}
sgj.Connection = conn

return sgj, nil
return nil
}

func (sgj *volumeGroupJournalConfig) Destroy() {
Expand Down