-
Notifications
You must be signed in to change notification settings - Fork 312
Conversation
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 think the mirror algorithm is fine, but the implementation has a few flaws. Since everything is a giant loop, none of the defers are running until the very end. Furthermore, for a very large N, we'll hit unnecessary timeouts due to lack of parallelization.
I'd like to suggest we:
-
Break apart the main HTTP handler into (at least) one testable function.
-
Leverage some parallelization (either primitively with goroutines or using a library like https://github.com/gammazero/workerpool) with a configurable max.
-
Add tests for
internal/mirror/mirror.go
. The blobstore should be stubbable to the in-memory implementation.
|
||
func deadlinePassed(deadline time.Time) bool { | ||
return time.Now().After(deadline) | ||
} |
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'd really like to see this broken down into smaller functions that are easily tested. The current HTTP handler is very long and very nested.
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.
can do as a follow up if that's OK
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.
yup
Parallelization is achieved at the HTTP layer - will have a cron to active this service every 60 seconds. if one mirror is locked and taking a while, the next invocation will move onto the next one. State is saved on an http request timeout and processing will continue on next invoke.
will do in follow up.
we can add additional parallelism, but doesn't seem to be necessary in the testing i've done.
would like to do in follow up |
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.
/lgtm
/hold
in case someone else wants to give it a once-over. Feel free to unhold.
|
||
func deadlinePassed(deadline time.Time) bool { | ||
return time.Now().After(deadline) | ||
} |
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.
yup
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikehelmick, sethvargo 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 |
/cancel hold |
/unhold |
Towards #928
Proposed Changes
Release Note