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

Jump to bottom

Add comments for singleInstance creation mechanism in giteaclient.go #534

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Add comments for singleInstance creation mechanism in giteaclient.go #534

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9b6aa1
remove dead code in giteaclient.go
kushnaidu Feb 26, 2024
c9d47a1
Add comments to code
kushnaidu Feb 28, 2024
df2894d
Update controllers/pkg/giteaclient/giteaclient.go
kushnaidu Feb 28, 2024
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
10 changes: 3 additions & 7 deletions controllers/pkg/giteaclient/giteaclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,9 @@ func GetClient(ctx context.Context, client resource.APIPatchingApplicator) (Gite
if singleInstance == nil {
lock.Lock()
defer lock.Unlock()
if singleInstance == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing implementation uses the check-lock-check pattern, The check in L59 is to determine whether we need to lock or not. Once lock is acquired, the check at L62 ensures some other thread has not created an instance between the time we did the check at L59 and acquired the lock. Hope this make sense and also why it is not dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh perfect, got it, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh perfect, got it, thanks!

@kushnaidu You may want to instead update the file with comments on why the code is structured the way it is, so that others may not accidentally think it is dead code. It will also be a good first PR contribution (only if it makes sense to you) from your side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @vjayaramrh, does it look better now?

singleInstance = &gc{client: client}
log.FromContext(ctx).Info("Gitea Client Instance created now.")
go singleInstance.Start(ctx)
} else {
log.FromContext(ctx).Info("Gitea Client Instance already created.")
}
singleInstance = &gc{client: client}
log.FromContext(ctx).Info("Gitea Client Instance created now.")
go singleInstance.Start(ctx)
} else {
log.FromContext(ctx).Info("Gitea Client Instance already created.")
}
Expand Down