add fleet missions queue #363 feature#817
Conversation
|
Hi @yazilimmz, thanks a lot for your work on making the fleet mission processing more robust! The ab8cc33 commit has just been merged and I have re-ran the tests. The custom php artisan race condition tests now however seem to fail. They probably need to be refactored a bit to account for the new system? I'll try and functionally test the changes once all actions succesfully run. |
jackbayliss
left a comment
jackbayliss
left a comment
There was a problem hiding this comment.
I've added some minor questions / feedback while on my travels 🫡
| // Update all fleet missions of player that are associated with any of the player's planets. | ||
| $player->updateFleetMissions(); | ||
|
|
||
| if (app()->environment('testing')) { |
There was a problem hiding this comment.
How come we're only doing this during testing? Would we not expect this in prod?
There was a problem hiding this comment.
We don't expect this in the production environment, because in production it runs through Laravel's queue system
There was a problem hiding this comment.
Is it possible to refactor the feature tests so they do use the Laravel's queue system, so we can assert that the queue processing etc. works using the existing test suite?
I'm not very familiar with Laravel's queue system, but as the feature tests hit the actual database, the queue system should also be able to work normally, right?
There was a problem hiding this comment.
It's possible to run assertions using Queue::fake();
So I'd expect to that that IMO.
👍
There was a problem hiding this comment.
We use travelTo to simulate fleet events. Unfortunately, with Redis queue, we cannot trigger a battle or a fleet event using travelTo. With $player->updateFleetMissions();, we are already testing the function inside the queue anyway.
lanedirt
left a comment
lanedirt
left a comment
There was a problem hiding this comment.
Hi @yazilimmz, I have tested the change locally, and functionally it seems to work great.
However one main thing I do notice is that mission processing now feels a bit sluggish, in the sense that after a mission has arrived, it takes 1-2 seconds before the mission is actually processed.
During this time the mission is not processed, it stays visible in the fleet mission event bar as "Finished". Even when you reload the page it can look like this for 1-2 seconds:
Due to this new delay, it might make sense that we do not show missions in the "Fleet movement" bar if the time_arrival in the past. I think the official OGame does this too. But we can do this in a follow up PR too.
One central question though: is there any possibility in making the queue system more responsive? E.g. even shaving off 0.5 seconds in the response time can make the game feel more snappy.
| REDIS_HOST=ogamex-redis | ||
| REDIS_PASSWORD=null | ||
| REDIS_PORT=6379 |
There was a problem hiding this comment.
This change here means all existing deployments who want to upgrade also need to change their .env, correct?
I tested this PR locally without making this .env change, but as the docker-compose.yml (dev) does expose a port it works via the old 127.0.0.1 connection. But on production deployments this won't work.
I'm thinking we may want to add some sort of initialize script somewhere during docker container start to check for and update default .env values. Such an initialization script does not exist yet, but it would be helpful for future (default) changes too. Because right now without instructions, if someone follows the current upgrade instructions in the README.md, the fleet queue will break.
There was a problem hiding this comment.
Using a script could overwrite the user's custom values. I think updating the README.md would be a better approach. Something like:
"If you are upgrading from version 0.13.0 or earlier, please add REDIS_HOST=ogamex-redis to your .env file."
Good catch! I've reduced the queue worker sleep time from the default 3 seconds to 0.5 seconds. This should make the queue much more responsive. |
|
@lanedirt Is there anything you expect from me for this PR? |
Looks good so far, thank you! I just haven’t had the time to properly test it again, and also deciding about best way to go for the upgrade notice or script. I’ll review it soon and get back to you. |
|
I have just tested it again on my local environment. I have two areas that could use some further thought: Redis persistenceRight now, Redis is by default configured with The scenario:
To combat this, I think setting the ResumabilityTwo scenarios will cause fleet missions to never be processed:
The FIFO check makes this worse: The current code waits for earlier unprocessed missions before processing later ones. So if mission A (arrival 10:00) has no Redis job, mission B (arrival 10:15) will keep releasing itself every second waiting for A, which will never come. I'm seeing this locally now actually: I have 295 orphaned missions (from lots of earlier tests) causing some of my new manual dispatched missions to stall indefinitely in a release(1) loop. E.g. this is what my ogamex-queue-worker log looks like right now, its trying to re-process the same job every second but gets stuck due to the FIFO check: laravel.log: So for this I think we'll need some kind of resume functionality, where it periodically checks for jobs that have arrived ( Can you look into this? If I made a wrong observation or assumption, feel free to correct me. Thanks again for your work on this! |
|
Thanks a lot for the detailed analysis. your observations make sense. Redis persistence: Resumability: Return-phase missions & long battle times FIFO: Since Redis cannot guarantee strict FIFO modifying the FIFO logic deeply may cause more issues (like endless release loops). |
Addresses issue lanedirt#363 by documenting a comprehensive plan for implementing a Redis-backed queue system that ensures fleet missions are processed in strict chronological order by arrival time. Key components: - Redis container and PHP extension setup - ProcessFleetMission job with FIFO ordering - Orphan mission recovery mechanism - Dual-mode operation for test compatibility - Migration path for existing deployments Based on analysis of current codebase and PR lanedirt#817 discussion.
Status note / handoffThis PR pioneered the Redis delayed-job approach for #363, but the remaining blockers from the Nov 28 review (Redis Suggested path forward: #1413 is a more complete take on the same issue that already solves those blockers (database queue + scheduler catch-up + destination locks + tests). A small hardening follow-up for timeout/sleep is in Geda173/OGameX#68. Unless there is a strong reason to keep the Redis-based design, closing this in favor of #1413 (and crediting the groundwork here) would probably reduce confusion for new contributors. |
4 participants
Description
Please include a summary of the changes made to OGameX and their purpose. Clearly describe what this PR does and why it is necessary.
Type of Change:
Related Issues
Fixes #363
Checklist
Before submitting this pull request, ensure all following requirements as outlined in CONTRIBUTING.md are met:
Additional Information
Aim for near real-time processing: missions should be executed within 1-3 seconds of their scheduled time.
Ensure scalability to handle multiple concurrent missions across the game universe. E.g. missions for different planets that do not conflict with each other can be processed concurrently and do not necessarily have to wait for each other.
Implement logging and error handling for mission processing.
Consider edge cases like server downtime or processing delays.
Additionally, testing was becoming too difficult. I added a command for this. You can run it like:
php artisan fleet:test-queue 5 1 347 12 --delay=5. This command will launch 3 simultaneous attacks to the coordinates you specify. You can also reduce this in terms of duration by increasing the battle speed from settings. You can remove this optionally.