-
Notifications
You must be signed in to change notification settings - Fork 1
Refactored tasks package into transfers package.
#162
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
dc434a7 to
4562caf
Compare
|
We're almost there. I just need to do a bit more testing of the new orchestration logic within the service, and then propagate the config data structure. |
1614b3e to
7c16405
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 64.06% 66.54% +2.48%
==========================================
Files 24 20 -4
Lines 2585 1949 -636
==========================================
- Hits 1656 1297 -359
+ Misses 754 542 -212
+ Partials 175 110 -65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I want to test this "in production" over the weekend before merging it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice! I just have a couple minor questions/comments, mostly about how goroutines interact.
What wasn't obvious to me at first is that successful staging triggers a move directly, and moves trigger manifests to be generated, and that all three directly set the status for the transfer. It all makes sense now, but I wonder if there is somewhere to put this info so it's easy for a newbie like me to get a picture of the overall workflow?
|
One thing I was curious about is why you set the channel buffers to 32? could there ever be more than one element in each channel? |
Good point. We should probably relate this to the number of allowed HTTP connections via a parameter. And I should go over the code and change the channel buffers to reflect the actual expected queue sizes. |
50f6857 to
c07c522
Compare
The transfers package uses concurrent sequential programs to divide up the labor in transferring files.
…rs messages. This mechanism is useful for testing and can also be wired into a message queue down the line.
c07c522 to
18a6882
Compare
|
Okay, I think this is ready to go--I've tested it in Spin with dtspy and with Ken Chu's IMG -> KBase push service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
|
||
| type dispatcherChannels struct { | ||
| RequestTransfer chan Specification // used by client to create a new transfer | ||
| ReturnTransferId chan uuid.UUID // returns task ID to client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ReturnTransferId chan uuid.UUID // returns task ID to client | |
| ReturnTransferId chan uuid.UUID // returns transfer ID to client |
| d.Channels.CancelTransfer <- transferId | ||
| err := <-d.Channels.Error | ||
| if err != nil { | ||
| slog.Error(fmt.Sprintf("Transfer %s: %s", transferId.String(), err.Error())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but if we log cancel errors, should we also log errors for other dispatcher functions?
| /* | ||
| dispatcherTests := DispatcherTests{Test: t} | ||
| print("dispatcher start/stop\n") | ||
| dispatcherTests.TestStartAndStop() | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed?
The
taskspackage is kind of a mess--it has a single goroutine that updates the states of all the transfers in a very cross-cutting way, and then it has another goroutine that provides a "heartbeat" to poke it for the next update.The
transferspackage that replaces it breaks up the work of file transfers into distinct goroutines with crisp areas of responsibility:prototype code in
services/) and coordinates with the other goroutinesI've used private package global variables for simple namespacing.
Closes #159