Add support for global rank parsing in /users/ batch lookups#36249
Add support for global rank parsing in /users/ batch lookups#36249bdach merged 1 commit intoppy:masterfrom
Conversation
peppy
added
the
area:online functionality
There was a problem hiding this comment.
I don't think this is a good idea. I see so many problems ahead if this is allowed to pass.
To offer something concrete and constructive I'd have to spend a few hours because yes APIUser is bad and righting it would take 2 weeks of time so suggesting that is unrealistic. I would probably attempt to not touch it at all or very lightly at best (maybe have a side endpoint just for looking up ranks without full profiles?). Same probably goes for the cache, as well - I would sooner consider just not using the memory cache in the usage concerned at all, or using a local instance thereof isolated from the game-global one, than go through with this.
osu.Game/Database/UserLookupCache.cs
Outdated
| Ruleset.BindValueChanged(ruleset => | ||
| { | ||
| if (ruleset.OldValue.OnlineID != ruleset.NewValue.OnlineID) | ||
| Clear(); | ||
| }); |
There was a problem hiding this comment.
I cannot describe how much I dislike this. Full purge of game-global cache on every global ruleset switch? That is presumably going to kneecap the cache and increase load on web. At that point is it even worth caching much?
That and also consider races. If you're going to choose to do this then every single cache lookup that's ongoing at the time of the ruleset switch should be immediately aborted and discarded lest you store data that is basically wrong.
Stateful caches are one of my nightmares. I would rather we didn't.
There was a problem hiding this comment.
I dunno. The overlay which is using this requests every online user when it opens, across something like 50-100 web requests. Without the cache, this happens every two F9 key presses. (unrelated: It's also receiving 100s of updates per second from server-spectator).
osu-web is already being made to handle this scenario because it's the only way we can get usernames for display on the currently online display.
It's already all shit. I guess the way forward is just closing this and the overlay update PR and rewriting everything to never depend on osu-web? Would require server-spectator to provide, at very least, user ranks in some presence update.
Another direction would be making UserLookupCache able to populate ruleset ranks one at a time.
There was a problem hiding this comment.
I'd be much more fine with this if you can somehow confine the blast radius of it to the overlay concerned. Even by doing something like
- having the cache instance be local to the overlay
- only accepting a single fixed ruleset in the ctor (or
null) - recycling the entire cache on ruleset change but only local to the overlay
- game-global cache just does not attempt to track any of this by passing
nullin the ctor
| [JsonProperty(@"global_rank")] | ||
| public GlobalRank Rank | ||
| { | ||
| set => Statistics.GlobalRank = value.Rank; |
There was a problem hiding this comment.
I'd honestly rather have this be plain get; set; than do this. Half of the data that the endpoint provides is moved elsewhere sans context and the other half (the aforementioned context) is just discarded so you can't even tell what ruleset the global rank is valid in.
|
I'll toy with a local |
Sounds much more palatable for sure. |
peppy
force-pushed
the
global-rank-shortcut-support
branch
from
f356ee0 to
311a8ae
Compare
| Ruleset.BindValueChanged(ruleset => | ||
| { | ||
| if (ruleset.OldValue?.OnlineID != ruleset.NewValue?.OnlineID) | ||
| Clear(); |
There was a problem hiding this comment.
This still isn't flushing / cancelling all in-flight lookups at the time of change.
There was a problem hiding this comment.
I'm probably going to pull that out of this PR altogether as it has moved files in the other PR and merging is a huge PITA.
There was a problem hiding this comment.
As a result this PR is kinda nothing. I can just close if preferred and add it to the 1029408712 loc overlay change
There was a problem hiding this comment.
Either's fine as far as I'm concerned 🤷
There was a problem hiding this comment.
Also I think this is way too high effort to make work properly. Invalidate suffers from the same issue. In flight tasks should just go as they are IMO.
There was a problem hiding this comment.
Let's see how long until an issue report of "user rank on currently playing overlay is from the wrong ruleset" then I guess? Unless you're planning on guarding against that somewhere else?
See ppy/osu-web#12651 for web-side implementation. To be used to fix https://github.com/ppy/osu/pull/33649/changes#diff-32784a778b34c671e1f72c00e1f4161a5e774e849aae5631ee71b31fc32e5d42R218 (have tested this works there). Use simple set-get rather than transforming to `APIUser.Statistics`
peppy
force-pushed
the
global-rank-shortcut-support
branch
from
7963fa8 to
23c68cb
Compare
github-project-automation
bot
moved this from Pending Review
to Done
in osu! team task tracker
2 participants
See ppy/osu-web#12651 for web-side
implementation.
To be used to fix https://github.com/ppy/osu/pull/33649/changes#diff-32784a778b34c671e1f72c00e1f4161a5e774e849aae5631ee71b31fc32e5d42R218
(have tested this works there).
Am reluctant about overloading
APIUserto handle this "slim" case, but I think splitting it out is going to be somewhat of a headache.