Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Dont hang outstanding jobs when blockstoreManager shuts down #239

Closed
wants to merge 2 commits into from

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jan 21, 2020

Fixes #237 by adding an error parameter to the job function. When the job is scheduled it first checks to see if there was a scheduling error before running.

@Stebalien
Copy link
Member

I'm concerned that this just drops the jobs then does nothing. How about #240?

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 22, 2020

I think #240 will still hang outstanding jobs on shutdown, because in the worker loop it returns on px.Closing() (and doesn't drain the queue)

I'm concerned that this just drops the jobs then does nothing

At present we just ignore errors (and don't add anything into the results map for CIDs that get an error), which is why I was thinking that it should be ok to do the same if the process is closing.

@Stebalien
Copy link
Member

I think #240 will still hang outstanding jobs on shutdown, because in the worker loop it returns on px.Closing() (and doesn't drain the queue)

It shouldn't, unless I'm missing something. There's no buffer so:

  • All in progress jobs should execute.
  • All unscheduled jobs should fail inside addJob.

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 23, 2020

Ah yeah I see that's true, I like this solution better.
Do you want me to remove the contexts as well?

@Stebalien
Copy link
Member

We can keep them for now. That's more of a future "this is tricky" concern.

@Stebalien Stebalien closed this Jan 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blockstoreManager.jobsPerKey is hangs if the context is canceled.
2 participants