Support queuing score by id #114

notbakaneko · 2022-07-15T09:36:04Z

Adds support for queuing solo_score for indexing by id.
The indexer will do the necessary lookups and index or delete the document as necessary; scores with missing user or beatmap will be deleted from the index.

The structure of SoloScore is specific to the indexer itself and probably not convenient for other clients to use, especially if the format changes.

To queue for indexing, push to the queue:

{ "ScoreId": 1 }

ScoreId in the queue item will have priority over Score (score gets ignored)

Added some utility commands:
To queue a score for indexing or deletion by id:

scores index ${id}

Pushing the contents of filename to the queue as a single item - this is mainly so I can test with arbitrary json blobs instead of the result of JsonConvert.SerializeObject:

push-file ${filename}

~~Also adds the convert flag to the schema but haven't added added the convert lookup yet.~~
Also does the convert lookup (assuming score's ruleset_id being different from beatmap's playmode)

closes #112

makes it easier to test handling arbitrary data instead of making different QueueItem subclasses

peppy · 2022-07-15T12:18:33Z

@notbakaneko to confirm, is this ready to go?

nanaya · 2022-07-15T12:42:46Z

The index action works as expected. Tested using my test branch.

notbakaneko · 2022-07-15T13:46:52Z

Noticed some strange errors earlier; it should work properly now - I'll move the parts that organize the items to index/delete out later

peppy · 2022-07-16T06:33:53Z

README.md

+### Deleting a score by `id`
+```json
+{ "Action": "delete", "ScoreId": 1 }
+```


What's the case for deleting? Wondering if we even need an action if the deletion case is always "doesn't exist in database or has preserve=0.

So a document can be deleted directly when testing for effects.

So it will never be used in actual usages?

No, it's a convenience when testing

I mean, considering it's going to be fetching from a mysql table at the end of the day (where you can set preserve:false) it's probably fine to remove here to keep things simple then?

Being able to just delete it makes it easier than needing to flip preserve between true and false on he db first; with the command I can just do score delete 1 to remove and score index 1 to put it back; it also means not needing an entire infrastructure and complete dataset to always be present and be in sync when testing 👀

I still don't get it. The indexer does lookups from the database. How do you test without data/infrastructure?

If this is really required, can we keep it on a dev branch, as I don't foresee it being useful in production?

peppy · 2022-07-16T06:34:57Z

osu.ElasticIndexer/Commands/PushFileCommand.cs

+            var redis = new Redis();
+            var database = redis.Connection.GetDatabase();
+            var queueName = $"osu-queue:score-index-{AppSettings.Schema}";
+
+            database.ListLeftPush(queueName, value);


Any reason for doing it like this rather than using the existing queue method? I'd much prefer the latter.

This makes it easier to push arbitrary structures into the queue for testing instead of it being serialized into a known type, or defining a new type and queue processor instance for every payload I want to check.

But you can just new ScoreItem { id = ... } no?

Pushing directly to the key allows pushing things like { "ScoreId": 1, "not_a_real_field": "aa" } and also intentionally omit properties that didn't exist in other versions, instead of it being serialized into to type of the current version, which isn't as useful when testing for payloads from external sources.

processor.PushToQueue(new ScoreItem()) is always going to push "{\"Action\":null,\"ScoreId\":null,\"Score\":null,\"TotalRetries\":0,\"Tags\":null}" to the queue; a different client isn't necessarily going to be sending all the fields.

I guess it's fine to leave this, since this command probably won't be used in production. Still a bit weird though. I can imagine this breaking if we change the queue name or system and forget to update this. shrug

…long`

peppy · 2022-07-20T10:06:48Z

@notbakaneko I've applied some changes and tested locally that things still seem to work. Of concern is 1e8f036 where I change the deletion operation to use longs directly, instead of strings. Let me know if there was a reason you didn't have it running this way (it seems to work as expected).

notbakaneko · 2022-07-20T13:14:32Z

osu.ElasticIndexer/IndexQueueProcessor.cs

@@ -86,23 +85,6 @@ private void addToBuffer(SoloScore score, ProcessableItemsBuffer buffer)
                buffer.Deletions.Add(score.id);
        }

-        private BulkResponse dispatch(BulkDescriptor bulkDescriptor)


this isn't supposed to be unused; apparently I managed to test it and somehow not push the change using it 👀

Did you want to add the usage back for this then?

I was going to update the calls to it but there was an issue I came across when checking ES8 support, except I can't seem to replicate it now 🤔

The new logic means that if ES goes away, the queue processor will never exit. I don't think this should be the case? Queue processor itself already has fail and retry logic, so unless this is intended to handle actual intermittent errors during normal operation, I'd either leave the handling to that mechanism, or make it retry for a max of 1-5 seconds to ensure things don't get stuck.

Retrying queue items with minimal delay can easily deplete all the retries while a server restarts or network recovers; a better way may be to be able to just re-queue an item without it being marked as failed?

Maybe a setting on QueueProcessor that ensures failed items are never dropped, for cases that matters.

Will merge as-is for now and we can revisit when this becomes an issue. Tracking at ppy/osu-queue-processor#16.

osu.ElasticIndexer/IndexQueueProcessor.cs

notbakaneko · 2022-07-20T13:30:19Z

change the deletion operation to use longs directly, instead of strings.

The original call was DeleteMany(ids) without specifying <T>which needed the type of be an object or string but there's currently a bug requiring T to be explicitly specified when using ids only.

peppy · 2022-07-20T13:37:30Z

Yeah I saw that, but seems longs work fine as long as the generic is specified.

peppy · 2022-07-22T04:01:36Z

osu.ElasticIndexer/IndexQueueProcessor.cs

@@ -86,23 +85,6 @@ private void addToBuffer(SoloScore score, ProcessableItemsBuffer buffer)
                buffer.Deletions.Add(score.id);
        }

-        private BulkResponse dispatch(BulkDescriptor bulkDescriptor)


The new logic means that if ES goes away, the queue processor will never exit. I don't think this should be the case? Queue processor itself already has fail and retry logic, so unless this is intended to handle actual intermittent errors during normal operation, I'd either leave the handling to that mechanism, or make it retry for a max of 1-5 seconds to ensure things don't get stuck.

notbakaneko added 15 commits July 13, 2022 18:04

add convert to schema

4bfce33

adding commands to index/delete specific score

5cb8623

figure out deserialization later

e9b8026

support looking up records by id

f8435d4

only need the id for deleting

e334397

consolidate to single command

0635423

Required long arugment just puts 0 if it's missing...

56caed6

move the Action/ScoreId out of the score structure

fc49a8d

add command for pushing arbitrary commands to queue.

12e5b7b

makes it easier to test handling arbitrary data instead of making different QueueItem subclasses

sending empty payload is an error

b40a6c6

helper method for organizing bulk descriptor payload

ec9d810

remove scores that don't exist via lookup

3c1450e

rely on ScoreItem.ToString()

dac0f58

add some documentation

e592b85

fix description

2c2840a

notbakaneko self-assigned this Jul 15, 2022

notbakaneko added 3 commits July 15, 2022 18:47

formatting

708546b

unused

8579baa

was missing the extra space before >_>

4118771

shouldn't be sharing the buffer between tasks

d7cfa68

peppy reviewed Jul 16, 2022

View reviewed changes

notbakaneko and others added 6 commits July 20, 2022 13:02

use existing queue name

4b995dc

remove direct deletion support

e0e829c

remove action

25a5fc6

more accurate condition of indexing in readme

3e601b7

Simplify ScoreItem construction

34af8ac

Rename IndexQueueItems to ProcessableItemsBuffer

13ccc1d

peppy added 4 commits July 20, 2022 19:05

Add xmldoc for ProcessableItemsBuffer and change deletions to use `…

1e8f036

…long`

Show deleted document count in list command

5745b00

Remove unused method

4756205

Tidy up remaining flow a touch

cd149e0

peppy previously approved these changes Jul 20, 2022

View reviewed changes

Merge branch 'master' into feature/queue-item-action

6887302

notbakaneko commented Jul 20, 2022

View reviewed changes

osu.ElasticIndexer/IndexQueueProcessor.cs Show resolved Hide resolved

handle network errors in dispatch instead of simply failing

0105420

notbakaneko dismissed peppy’s stale review via 0105420 July 21, 2022 11:25

add convert lookup

8af1705

peppy requested changes Jul 22, 2022

View reviewed changes

peppy approved these changes Jul 22, 2022

View reviewed changes

peppy merged commit 8a3b5f7 into ppy:master Jul 22, 2022

notbakaneko mentioned this pull request Jul 27, 2022

Convert scores may need to be marked #111

Closed

notbakaneko deleted the feature/queue-item-action branch August 30, 2022 04:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support queuing score by id #114

Support queuing score by id #114

notbakaneko commented Jul 15, 2022 •

edited

Loading

peppy commented Jul 15, 2022

nanaya commented Jul 15, 2022 •

edited

Loading

notbakaneko commented Jul 15, 2022

peppy Jul 16, 2022

notbakaneko Jul 19, 2022

peppy Jul 19, 2022

notbakaneko Jul 19, 2022

peppy Jul 19, 2022

notbakaneko Jul 19, 2022

peppy Jul 19, 2022

peppy Jul 16, 2022

notbakaneko Jul 19, 2022

peppy Jul 19, 2022

notbakaneko Jul 19, 2022

peppy Jul 19, 2022

peppy commented Jul 20, 2022

notbakaneko Jul 20, 2022

peppy Jul 20, 2022

notbakaneko Jul 21, 2022

peppy Jul 22, 2022

notbakaneko Jul 22, 2022

peppy Jul 22, 2022

peppy Jul 22, 2022

notbakaneko commented Jul 20, 2022

peppy commented Jul 20, 2022

peppy Jul 22, 2022

Support queuing score by id #114

Support queuing score by id #114

Conversation

notbakaneko commented Jul 15, 2022 • edited Loading

peppy commented Jul 15, 2022

nanaya commented Jul 15, 2022 • edited Loading

notbakaneko commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notbakaneko commented Jul 20, 2022

peppy commented Jul 20, 2022

Choose a reason for hiding this comment

notbakaneko commented Jul 15, 2022 •

edited

Loading

nanaya commented Jul 15, 2022 •

edited

Loading