Add comments for singleInstance creation mechanism in giteaclient.go #534
Conversation
Signed-off-by: Kushal Harish Naidu <[email protected]>
|
/approve |
kushnaidu
requested review from
liamfallon,
efiacor and
Catalin-Stratulat-Ericsson
no problem :D |
|
@vjayaramrh are you ok ? |
| @@ -59,13 +59,9 @@ func GetClient(ctx context.Context, client resource.APIPatchingApplicator) (Gite | |||
| if singleInstance == nil { | |||
| lock.Lock() | |||
| defer lock.Unlock() | |||
| if singleInstance == nil { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh perfect, got it, thanks!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sure, I'll do that.
There was a problem hiding this comment.
Hey @vjayaramrh, does it look better now?
@henderiw FYI I have commented in the PR why we should keep the code the way it is |
|
Thx @vjayaramrh .agreed, thx for looking at this. I will close the PR. |
|
Reopened so that @kushnaidu can add the comment suggested by @vjayaramrh . |
|
@kushnaidu can you provide the comments for readability ? |
Yes yes, I shall do that. |
will do it now. |
Signed-off-by: Kushal Harish Naidu <[email protected]>
kushnaidu
changed the title
| @@ -55,10 +55,12 @@ func GetClient(ctx context.Context, client resource.APIPatchingApplicator) (Gite | |||
| if client.Client == nil { | |||
| return nil, fmt.Errorf("failed creating gitea client, value of client.Client cannot be nil") | |||
| } | |||
|
|
|||
| // check if instance is created | |||
There was a problem hiding this comment.
you may want to mention "check-lock-check pattern implementation is followed" to make it easy for folks to search about the pattern
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: henderiw, liamfallon, vjayaramrh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ephio-project#534) This change removes a conditional statement which is never executed. --------- Signed-off-by: Kushal Harish Naidu <[email protected]>
This change removes a conditional statement which is never executed.