-
Notifications
You must be signed in to change notification settings - Fork 3
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
Make task queues configurable #655
Conversation
6962224
to
aeac203
Compare
aeac203
to
f3b1e36
Compare
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.
I've suggested a number of minor modifications. I realize there are quite a few and I hope it's not overwhelming.
c91465a
to
036deec
Compare
Hey, this is ready to review again. I added all of your suggestions. |
There is a failing test:
|
34128fc
to
0081cc6
Compare
It works! |
1fce59b
to
9592b11
Compare
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.
A few small suggestions/questions.
go.mod
Outdated
golang.org/x/sys v0.13.0 // indirect | ||
golang.org/x/text v0.13.0 // indirect | ||
golang.org/x/time v0.3.0 // indirect | ||
golang.org/x/tools v0.13.0 // indirect | ||
golang.org/x/tools v0.14.0 // indirect |
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.
It'd be nice if you don't include unrelated changes to go.mod in your commit if they're not strictly related.
Just realized this is ready for another review, will do tomorrow! Sorry for the late response. |
Perfectly fine. Should have sent you a DM. |
0f3b50f
to
60ee7f9
Compare
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.
GitHub won't allow me to comment on the specific line, but the storage.Submit
method (L117) was not updated to pass the task queue to the upload workflow request. I had to make this change:
- _, err = InitStorageUploadWorkflow(ctx, s.tc, &StorageUploadWorkflowRequest{AIPID: aipID})
+ _, err = InitStorageUploadWorkflow(ctx, s.tc, &StorageUploadWorkflowRequest{
+ AIPID: aipID,
+ TaskQueue: s.config.TaskQueue,
+ })
Other than that, 👍. I'll merge, thank you!
This link shows the changes I had to make but it's confusing because it also includes the rebase work I did in preparation for the merge: compare:0f3b/60ee. The relevant bits are in internal/storage/service.go
and internal/storage/service_test.go
in case you're interested.
refs #621