Conversation
AlexisMontagne
requested review from
pauloestrella1994 and
xgoffin
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
xgoffin
left a comment
xgoffin
left a comment
There was a problem hiding this comment.
Not sure about it all but nice to ditch that much code
go.mod
Outdated
| @@ -5,27 +5,6 @@ go 1.23.0 | |||
| toolchain go1.23.1 | |||
group/map.go
Outdated
| k := k | ||
|
|
|
|
||
| const apiURL = "https://www.ecb.europa.eu/stats/eurofxref/eurofxref-daily.xml" | ||
|
|
||
| type RateFetcher struct { |
There was a problem hiding this comment.
Don't we want to delete this one? Since we have umoney now.
There was a problem hiding this comment.
I thought so too but it is used as a fallback in umoney apparently https://github.com/upfluence/umoney/blob/7de3a2619ac667b91d5ecf3fbd97812a433efa45/client/rate_fetcher.go#L10
There was a problem hiding this comment.
Probably better to move it there then
There was a problem hiding this comment.
id rather keep it here, as it only relies on the stdlib and umoney is private (and not this one). This can be used off the shelve in any situation
| } | ||
|
|
||
| func (b *Balancer) Get(ctx context.Context, opts balancer.GetOptions) (peer.Peer, error) { | ||
| func (b *Balancer[T]) Get(ctx context.Context, opts balancer.GetOptions) (T, func(error), error) { |
There was a problem hiding this comment.
func(error) is either nil or does nothing. Is it needed ?
There was a problem hiding this comment.
Yep not quite yet, as i was messing with the interface better do it all the way 😅. My goal is to make loadbalancer error aware. Ideally LB policies that can reduce the occurrence of a peer picking if it becomes faulty.
The goal of this PR was only to prepare for a new version: Break interfaces & delete old code. Not add new one
group/map.go
Outdated
| var v, err = fn(ctx, k) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
| return err | |
| return errors.WithStack(err) |
group/typed_group.go
Outdated
| func (tg *TypedGroup[T]) Do(fn TypedRunner[T]) { | ||
| tg.Group.Do(func(ctx context.Context) error { | ||
| fn, err := fn(ctx) |
There was a problem hiding this comment.
| func (tg *TypedGroup[T]) Do(fn TypedRunner[T]) { | |
| tg.Group.Do(func(ctx context.Context) error { | |
| fn, err := fn(ctx) | |
| func (tg *TypedGroup[T]) Do(runner TypedRunner[T]) { | |
| tg.Group.Do(func(ctx context.Context) error { | |
| fn, err := runner(ctx) |
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/ready-to-merge
reviewed/needs-work
labels
syncutil/keyed_singleflight.go
Outdated
| } | ||
|
|
||
| for _, e := range es { | ||
| e := e |
upfluence-bot
added
reviewed/ready-to-merge
reviewed/needs-work
and removed
reviewed/needs-work
reviewed/ready-to-merge
labels
| } | ||
| } | ||
|
|
||
| return v, ok, nil |
There was a problem hiding this comment.
Bug: Cache Error Handling Causes Missinterpretation
The Get method now incorrectly returns ok=false whenever an error occurs, whether from the underlying cache or the eviction policy operation. Previously, ok indicated if the item was found in the cache, independent of other errors. This change can cause callers to misinterpret cache hits (with an error) as cache misses.
5 participants
What does this PR do?
Prepare the v2 version of this repository.
There is two goals for this v2.
Simplifying the dependency graph. Over the years this repository has became a hodge podge of multiple library all stitched together and creating a dependency hells for libraries depending on it (looking at you lock/etcd). From now on this repo will only depend on the std lib, golang.org/x pkgs and our standard packages (upfuence/log,stats,cfg,errors)
Using generics, certains data structures were really crying to be using generics, looking at the
cacheinterface mainly. But not only singleflights, balancer, resolver and other iopool will benefit from it.*Clean up junk from the last 10 years. Some libraries were not used anymore, a new major version is a good time to do some spring cleaning.
What are the observable changes?
Good PR checklist
Additional Notes