Rework locked maps syncing #441
Conversation
* Do not create new views for maps from current world * Fix maps in shulkers not converting * Add bundle support for map conversion
There was a problem hiding this comment.
Hey - massive thanks for your work here, what a lovely late Christmas present for this project!
I've yet to take the chance to deep-dive into this just yet, but I've just had a read through of your implementation code - and it's very nicely done. Thanks for adhering to my style, makes my job a lot easier :). Appreciate your other Postgres PR fix as well, that's a silly mistake from me so I've merged that straight away.
Before I deep dive into this further, some points to address:
- I'd like to bring this feature to the Fabric side of HuskSync for parity, but unfortunately the concept of World UIDs... doesn't exist on Fabric! Unfortunately the UID.dat is entirely a CraftBukkit API concept. With my other Fabric-supporting mod, HuskHomes, I sort of end up having to create random UUIDs for worlds, which gets a bit messy. I think instead of binding world UUIDs, let's use world name + server name combo, adding another element to the compound primary key here.
- Since we're introducing additional database-side storage, we should consider as a priority having administration commands to manage & prune this data. Perhaps not as part of this PR, but certainly something to consider sooner than later.
- We should consider running the map canvas byte array data through snappy compression to reduce filesizes.
- A few naming & style conventions that I'll go into more thoroughly in a proper review.
- instead of "connect" in connectMapIds, how about "bind" -- just avoids conflating this with database connection stuff
- where we have multiple PreparedStatements in database methods, consider branching these off to a private function just to avoid overly long methods in the database implementation
I'll follow along with some more notes in due course, but hopefully this is good to get you started :)
As I think about it again, do we actually need world names here? The map id is bound to the server after all. |
Don't think we do actually, yeah. Map IDs are server-wide; a server - map ID mapping should be sufficient & probably simpler. |
Use server names instead of world UUIDs
Use server names instead of world UUIDs
|
Possible enhancement: add DB indexes |
|
Very happy to approve this, thank you very much for your work! Please feel free to commit your name to the HuskSyncCommand aboutmenu class and I'll merge this when I have time soon. |
There was a problem hiding this comment.
Hey @solaluset -- I've been having a bit more of a think about this. Firstly, sorry to drag this back to the workbench, but something was bothering me recently when I came close to merging --
We're introducing more DB reads! In essence this PR would somewhat defeat the object of using Redis to nicely cache data, as we're slowing the sync process to do DB reads, which could get expensive quickly.
So I think what I'd like to see as a final step (as the code is otherwise super lovely and acceptable here) is to introduce Redis caching for map points; always write to Redis with a reasonable expiry time, and read from Redis first falling back to database if not present.
Once again, very sorry for being a pain at the last minute. Thoughts on this?
First of all, thanks for this amazing plugin! And I personally think that map syncing feature is great too. However, I found some disadvantages of current implementation.
This PR tries to address these issues by creating two additional tables, one saves map data, the other saves old and new world and map IDs to properly convert them.
The code was tested with PostgreSQL and MySQL.