ppy / osu-infrastructure

40 stars 5 forks source link

Improving multiplayer things #24

Closed peppy closed 11 months ago

peppy commented 11 months ago

I've been working through migration to the new multiplayer_score_links table, along with review on https://github.com/ppy/osu/pull/24697 / https://github.com/ppy/osu-server-spectator/pull/185. I've also been in discussion with @nanaya over my slight unhappiness (inability to easily comprehend) the current structure of things.

To recap:

CREATE TABLE `multiplayer_score_links`
(
    `id`               bigint unsigned    NOT NULL AUTO_INCREMENT,
    `user_id`          int unsigned       NOT NULL,
    `room_id`          bigint unsigned    NOT NULL,
    `playlist_item_id` bigint unsigned    NOT NULL,
    `beatmap_id`       mediumint unsigned NOT NULL,
    `build_id`         mediumint unsigned NOT NULL DEFAULT '0',
    `score_id`         bigint unsigned             DEFAULT NULL,
    `created_at`       timestamp          NULL     DEFAULT NULL,
    `updated_at`       timestamp          NULL     DEFAULT NULL,
    PRIMARY KEY (`id`),
    KEY `multiplayer_score_links_score_id_index` (`score_id`),
    KEY `multiplayer_score_links_room_id_user_id_index` (`room_id`, `user_id`),
    KEY `multiplayer_score_links_playlist_item_id_index` (`playlist_item_id`),
    KEY `multiplayer_score_links_user_id_index` (`user_id`)
)

multiplayer_score_links is a table that replaces multiplayer_scores, and allows the majority of the score metadata to be stored in the main solo_scores table. While not immediately obvious from the structure or naming, it currently serves a dual purpose:

This is weird. So my proposal is that we stop using this table for the token process, and use solo_score_tokens instead.

CREATE TABLE `solo_score_tokens`
(
    `id`         bigint unsigned NOT NULL AUTO_INCREMENT,
    `score_id`   bigint               DEFAULT NULL,
    `user_id`    bigint          NOT NULL,
    `beatmap_id` mediumint       NOT NULL,
    `ruleset_id` smallint        NOT NULL,
    `build_id`   mediumint unsigned   DEFAULT NULL,
    `created_at` timestamp       NULL DEFAULT NULL,
    `updated_at` timestamp       NULL DEFAULT NULL,
    PRIMARY KEY (`id`)
)

To make this work, we should add a playlist_item_id NULL DEFAULT NULL to solo_score_tokens. This table is already ephemeral data so adding extra columns like this is not a huge deal. We can then restructure the multiplayer_score_links table to be used specifically for lookup purposes.

The new tables would look something like this:

CREATE TABLE `solo_score_tokens`
(
    `id`               bigint unsigned NOT NULL AUTO_INCREMENT,
    `score_id`         bigint               DEFAULT NULL,
    `user_id`          bigint          NOT NULL,
    `beatmap_id`       mediumint       NOT NULL,
    `ruleset_id`       smallint        NOT NULL,
    `playlist_item_id` bigint unsigned NULL DEFAULT NULL,
    `build_id`         mediumint unsigned   DEFAULT NULL,
    `created_at`       timestamp       NULL DEFAULT NULL,
    `updated_at`       timestamp       NULL DEFAULT NULL,
    PRIMARY KEY (`id`)
);

CREATE TABLE `playlist_scores`
(
    `playlist_item_id` bigint unsigned NOT NULL,
    `score_id`         bigint unsigned DEFAULT NULL,
    `user_id`          int unsigned    NOT NULL,
    PRIMARY KEY (`score_id`),
    KEY `multiplayer_score_links_score_id_index` (`playlist_item_id`),
    KEY `multiplayer_score_links_user_id_index` (`user_id`)
)

This ill make https://github.com/ppy/osu/pull/24697 / https://github.com/ppy/osu-server-spectator/pull/185 obsoleted. It would fix my naming issues with the multiplayer_score_links table.

Also some remaining cleanup tasks

Some other considerations

cc/ @nanaya @notbakaneko @bdach @smoogipoo

nanaya commented 11 months ago

dropping extra columns in score links table (and rename etc) is probably easier migration path.

bdach commented 11 months ago

I know it will likely sound like cope / sunk cost fallacy talking, but is solo_score_tokens being used for multiplayer like that not also weird? The playlist_item_id extra foreign key column could be possibly stretched as "fine", since playlists aren't far from solo play --- were it not for the fact that realtime multiplayer - which is not really playlists or "solo" in any way, shape, or form - is also going to be using it...?

I'm not arguing for saving my work don't get me wrong, but moreso, if we're fine with making this many sweeping changes, then I'm not sure this goes far enough? Like a rename of solo_score_tokens for score_tokens may be in order (because what's "solo" about them anymore?). I'm not even sure what the solo in solo_scores is for at this point either, if everything is just going to land there.

peppy commented 11 months ago

dropping extra columns in score links table (and rename etc) is probably easier migration path.

Yep, that was the intention.

The playlist_item_id extra foreign key column could be possibly stretched as "fine", since playlists aren't far from solo play --- were it not for the fact that realtime multiplayer - which is not really playlists or "solo" in any way, shape, or form - is also going to be using it...?

If we don't want to pollute the table like this, I did propose a context JSON column originally. But in the interest of keeping things simple (and the fact that the data is ephemeral for likely ~1 day max) I think a column works better for now.

Also consider that realtime multiplayer does use playlist_items too.

Like a rename of solo_score_tokens for score_tokens may be in order (because what's "solo" about them anymore?

See the checkbox footnote 😄

bdach commented 11 months ago

If we don't want to pollute the table like this, I did propose a context JSON column originally. But in the interest of keeping things simple (and the fact that the data is ephemeral for likely ~1 day max) I think a column works better for now.

If solo_score_tokens becomes score_tokens I instantly have 0 issue with the extra column + foreign key, because that removes any weird cross-domain concerns (the tokens are for scores, any scores, period).

notbakaneko commented 11 months ago

I agree with all tokens going into the tokens table and keeping the x_links for extra information that doesn't fit in solo_scores, the half and half thing going on now seems strange considering we've been trying to consolidate the score tables? 🤔

bdach commented 11 months ago

Question, important for client: what structure are endpoints like PUT /api/v2/rooms/{roomId}/playlist/{playlistItemId}/scores/{scoreId} going to return with this proposal? Is it solo_scores (or scores I guess, if we do that rename)? Or is it going to be multiplayer_score_links still?

If it's the former, no consideration needs to be made for client; if the latter, I'd still like the response to contain a "solo" score ID.

peppy commented 11 months ago

In my mind, mutliplayer_score_links should only be for internal use, and never seen by the client or user. @nanaya please confirm.

nanaya commented 11 months ago

Both already have more or less the same structure. The new id returned will be just score id including for multiplayer scores. Also the multiplayer score ones will have additional attributes like playlist item and room id.

peppy commented 11 months ago

The main portion of this change has now been deployed.

We will need to follow up with renaming multiplayer_score_links to playlist_scores and various other cleanup of unused columns (checklist updated in OP).

peppy commented 11 months ago

I've added VIEW aliases:

scores -> solo_scores playlist_scores -> multiplayer_score_links

This means that osu-web (and other projects) can update to using this new naming. I'll switch the actual tables with the aliases at some point in the near future.

CREATE VIEW playlist_scores AS SELECT * FROM multiplayer_score_links;
CREATE VIEW scores AS SELECT * FROM solo_scores;
bdach commented 11 months ago

What is going to happen with the other solo_ tables?

MySQL [osu]> SELECT table_name FROM information_schema.tables WHERE table_name LIKE 'solo_%' AND TABLE_SCHEMA = 'osu';
+-----------------------------+
| TABLE_NAME                  |
+-----------------------------+
| solo_score_tokens           |
| solo_scores                 |
| solo_scores_legacy_id_map   |
| solo_scores_performance     |
| solo_scores_process_history |
+-----------------------------+

Are they getting a rename to remove the solo_ prefix too?

peppy commented 11 months ago

Yeah, i'd say they will all follow suit, with aliases to ease migration.

We might want to standardise plurals while we're there?

solo_scores_legacy_id_map -> score_legacy_id_map solo_scores_performance -> score_performance solo_scores_process_history -> score_process_history

bdach commented 11 months ago

Mostly asking because I was gonna go get osu-server-spectator and osu-queue-score-statistics, but they use the other tables too so not sure it's worth doing this in stages, maybe best to get the aliases in place first...?

peppy commented 11 months ago

Yep, need osu-web to move first. No hurry.

nanaya commented 11 months ago

btw playlist_scores doesn't follow the other multiplayer related tables' multiplayer_ prefix. And since it's between (multiplayer_)playlist_items and scores it should be multiplayer_playlist_item_scores instead? There's no playlist after all and the equivalent would be (multiplayer_)room.

peppy commented 11 months ago

sure, let's go with multiplayer_listlist_item_scores.

So far:

CREATE VIEW scores AS SELECT * FROM solo_scores;
CREATE VIEW score_tokens AS SELECT * FROM solo_score_tokens;
CREATE VIEW score_legacy_id_map AS SELECT * FROM solo_scores_legacy_id_map;
CREATE VIEW score_performance AS SELECT * FROM solo_scores_performance;
CREATE VIEW score_process_history AS SELECT * FROM solo_scores_process_history;

CREATE VIEW multiplayer_playlist_item_scores AS SELECT * FROM multiplayer_score_links;
peppy commented 11 months ago

In the interest of closing this issue, I think the final steps are