-
Notifications
You must be signed in to change notification settings - Fork 97
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
Break BaseService
functions into serviceutil
#536
Conversation
ed8b822
to
7235b00
Compare
@@ -59,9 +59,6 @@ jobs: | |||
- name: Display Go version | |||
run: go version | |||
|
|||
- name: Install dependencies | |||
run: go get -t ./... |
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 workspace side effect: adding a new package to rivershared seems to make it hard to resolve by go get -t
despite go test
and another commands working fine. No functional change by removing this since all Go commands will get dependencies, but another strike against workspaces, and a weird one.
I was going through trying to refactor the demo project to use `rivershared`, and while doing so, noticed that it'd broken out some of the `BaseService` functions like `CancellableSleep` into a new package so that they could be used without an available `BaseService`. Looking over this API again, there's no good reason for functions like `CancellableSleep` and `ExponentialBackoff` to be tied so tight to `BaseService`. The reason they are is that `BaseService` provides a random source that both can conveniently use, but that can be moved to a function argument instead. The reason that `BaseService` has a random source is to avoid the old `math/rand` seeding problem. A single random source is initialized and seeding securely once, then placed onto an archetype that's inherited by all `BaseService` instances. Soon, even that can go away. One of the biggest things fixed by Go 1.22's `math/rand/v2` is that it's no longer necessary to seed the top level functions. Once we drop support for Go 1.21, we'll be able to simplify this code dramatically by dropping `BaseService.Rand` and the random source arguments from functions like `ExponentialBackoff` in favor of just using top level `math/rand/v2` functions. I also drop the variant `CancellableSleepBetween` in favor of having callers use `CancellableSleep` combined with `randutil.DurationBetween`, a new random helper similar to `IntBetween`. With all this in, we'll be able to fully jettison all utilities from the demo project in favor of going all in with `rivershared`'s equivalents.
7235b00
to
8f6ec6c
Compare
@bgentry With these changes I think we can get the demo app fully switched over to rivershared. |
thx. |
I was going through trying to refactor the demo project to use `rivershared`, and while doing so, noticed that it'd broken out some of the `BaseService` functions like `CancellableSleep` into a new package so that they could be used without an available `BaseService`. Looking over this API again, there's no good reason for functions like `CancellableSleep` and `ExponentialBackoff` to be tied so tight to `BaseService`. The reason they are is that `BaseService` provides a random source that both can conveniently use, but that can be moved to a function argument instead. The reason that `BaseService` has a random source is to avoid the old `math/rand` seeding problem. A single random source is initialized and seeding securely once, then placed onto an archetype that's inherited by all `BaseService` instances. Soon, even that can go away. One of the biggest things fixed by Go 1.22's `math/rand/v2` is that it's no longer necessary to seed the top level functions. Once we drop support for Go 1.21, we'll be able to simplify this code dramatically by dropping `BaseService.Rand` and the random source arguments from functions like `ExponentialBackoff` in favor of just using top level `math/rand/v2` functions. I also drop the variant `CancellableSleepBetween` in favor of having callers use `CancellableSleep` combined with `randutil.DurationBetween`, a new random helper similar to `IntBetween`. With all this in, we'll be able to fully jettison all utilities from the demo project in favor of going all in with `rivershared`'s equivalents.
I was going through trying to refactor the demo project to use
rivershared
, and while doing so, noticed that it'd broken out someof the
BaseService
functions likeCancellableSleep
into a newpackage so that they could be used without an available
BaseService
.Looking over this API again, there's no good reason for functions like
CancellableSleep
andExponentialBackoff
to be tied so tight toBaseService
. The reason they are is thatBaseService
provides arandom source that both can conveniently use, but that can be moved to a
function argument instead.
The reason that
BaseService
has a random source is to avoid the oldmath/rand
seeding problem. A single random source is initialized andseeding securely once, then placed onto an archetype that's inherited
by all
BaseService
instances.Soon, even that can go away. One of the biggest things fixed by Go
1.22's
math/rand/v2
is that it's no longer necessary to seed the toplevel functions. Once we drop support for Go 1.21, we'll be able to
simplify this code dramatically by dropping
BaseService.Rand
and therandom source arguments from functions like
ExponentialBackoff
infavor of just using top level
math/rand/v2
functions.I also drop the variant
CancellableSleepBetween
in favor of havingcallers use
CancellableSleep
combined withrandutil.DurationBetween
,a new random helper similar to
IntBetween
.With all this in, we'll be able to fully jettison all utilities from the
demo project in favor of going all in with
rivershared
's equivalents.