Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support queuing score by id #114
Support queuing score by id #114
Changes from 29 commits
4bfce33
5cb8623
e9b8026
f8435d4
e334397
0635423
56caed6
fc49a8d
12e5b7b
b40a6c6
ec9d810
3c1450e
dac0f58
e592b85
2c2840a
708546b
8579baa
4118771
d7cfa68
4b995dc
e0e829c
25a5fc6
3e601b7
34af8ac
13ccc1d
1e8f036
5745b00
4756205
cd149e0
6887302
0105420
8af1705
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't supposed to be unused; apparently I managed to test it and somehow not push the change using it 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to add the usage back for this then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a setting on
QueueProcessor
that ensures failed items are never dropped, for cases that matters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge as-is for now and we can revisit when this becomes an issue. Tracking at ppy/osu-queue-processor#16.