Skip score uploads when nothing was hit in score #224
Conversation
The client will not submit a play if hit statistics indicate that they never hit anything: https://github.com/ppy/osu/blob/a47ccb8edd2392258b6b7e176b222a9ecd511fc0/osu.Game/Screens/Play/SubmittingPlayer.cs#L281 This check was not exercised spectator server-side, which meant that scores with nothing hit would get completed and enqueued for upload that would never succeed (because the score would never be submitted anyway). To save on some processing, just port the same check server-side to skip upload if nothing was hit. I'm hoping the effects of this is going to be the decrease of the score timeouts monitoring on sentry & datadog closer to real levels (see discussion in ppy#220).
| // note that this isn't an early return as we still want to end the play session. | ||
| // see: https://github.com/ppy/osu/blob/a47ccb8edd2392258b6b7e176b222a9ecd511fc0/osu.Game/Screens/Play/SubmittingPlayer.cs#L281 |
There was a problem hiding this comment.
I don't quite understand what this comment means by "end the play session" - there's nothing running after the block other than the finally, right?
There was a problem hiding this comment.
There is actually:
Skipping that causes a test to fail. I considered moving that call inside finally but wasn't convinced that was correct.
There was a problem hiding this comment.
Oh, it was hidden. How come that isn't an issue for the above return? Something somewhere (osu-web?) blocks it from starting?
There was a problem hiding this comment.
I can understand this one skipping it:
osu-server-spectator/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
Lines 138 to 141 in cc1d36d
because it assumes that BeginPlaySession() failed. But I don't understand this one:
osu-server-spectator/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
Lines 143 to 146 in cc1d36d
I think that may be an actual bug?
There was a problem hiding this comment.
Hmmm okay. That second one is the one I was referring to specifically. I think this is fine to get in as-is but that should probably be investigated.
There was a problem hiding this comment.
Yeah I'm looking at it.
…ranked beatmap plays As discussed in ppy#224 (comment). In full-stack testing, this manifests as an actual bug wherein when spectating a player who's playing an unranked beatmap, spectator player will not exit even when the spectated player is done playing and will hang there forevermore.
…ranked beatmap plays As discussed in ppy#224 (comment). In full-stack testing, this manifests as an actual bug wherein when spectating a player who's playing an unranked beatmap, spectator player will not exit even when the spectated player is done playing and will hang there forevermore.
RFC.
The client will not submit a play if hit statistics indicate that they never hit anything.
This check was not exercised spectator server-side, which meant that scores with nothing hit would get completed and enqueued for upload that would never succeed (because the score would never be submitted anyway). To save on some processing, just port the same check server-side to skip upload if nothing was hit.
I'm hoping the effects of this is going to be the decrease of the score timeouts monitoring on sentry & datadog closer to real levels (see discussion in #220).