Issue 7917: Fix/mfs/error pin fail#8525
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
|
@lidel : I know we have this marked as P3. I'm putting this on the board for now, but feel free to remove if you don't think you'll realistically get to it during PR Fridays. |
gammazero
left a comment
gammazero
left a comment
There was a problem hiding this comment.
There are too many issues with this PR, so converting it to draft.
|
|
||
| func checkPinStatus(ctx context.Context, node pinMFSNode, svcName string, svcConfig config.RemotePinningService, ps pinclient.PinStatusGetter, | ||
| repinInterval time.Duration) { | ||
| result := make(chan int) |
There was a problem hiding this comment.
There is no reader for this channel, so the goroutine will block forever when trying to write to it.
| result := make(chan int) | ||
| go func() { | ||
| for { | ||
| if ps.GetStatus() != pinclient.StatusFailed && ps.GetStatus() != pinclient.StatusPinned { |
There was a problem hiding this comment.
This should be the condition of the for loop, not a separate if. Then the else becomes unnecessary.
| go func() { | ||
| for { | ||
| if ps.GetStatus() != pinclient.StatusFailed && ps.GetStatus() != pinclient.StatusPinned { | ||
| time.Sleep(repinInterval) |
There was a problem hiding this comment.
Need to use a time.Timer here, so that select can also wait for context cancelation as well as timeout.
| } | ||
| if(ps.GetStatus() == pinclient.StatusFailed) { | ||
| mfslog.Errorf("pinning to %q failed", svcName) | ||
| result <- 0 |
There was a problem hiding this comment.
Where are these results used? If they are, a bool type is more appropriate and an int with a 0 or 1 value.
| } else { | ||
| var err error | ||
| repinInterval, err = time.ParseDuration(svcConfig.Policies.MFS.RepinInterval) | ||
| repinInterval, err = time.ParseDuration(svcConfig.Policies.MFS.RepinInterval) |
There was a problem hiding this comment.
Run go fmt to remove trailing whitespace, etc.
| //Run a goroutine to monitor status of queued pins | ||
| if ps.GetPin().GetCid() == cid && ps.GetStatus() == pinclient.StatusQueued { | ||
| checkPinStatus(ctx, node, svcName, svcConfig, ps, repinInterval) | ||
| } |
There was a problem hiding this comment.
I do not think we want to start a new goroutine for each pin. I think it would be better to communicate pins to watch to a single goroutine.
| break | ||
| } | ||
| //Run a goroutine to monitor status of queued pins | ||
| if ps.GetPin().GetCid() == cid && ps.GetStatus() == pinclient.StatusQueued { |
There was a problem hiding this comment.
The code above already does if ps.GetPin().GetCid() == cid so this and the above should be under a that single if condition.
| pinTime := time.Now().UTC() | ||
| pinStatusMsg := "pinning to %q: received pre-existing %q status for %q (requestid=%q)" | ||
| for ps := range lsPinCh { | ||
| log.Errorf("PS Typ eis is : %T", ps) |
There was a problem hiding this comment.
Should not log error here.
6 participants
Fixes #7917
Approach:
If the status of a pin is "queued", a goroutine will run for each pin until the status of that pin becomes "Pinned" or "Failed"