Skip to content
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

LevelDB player data storage #5467

Closed
wants to merge 4 commits into from
Closed

LevelDB player data storage #5467

wants to merge 4 commits into from

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Dec 23, 2022

Introduction

This PR implements a LevelDB player data storage provider.

Relevant issues

I previously wrote about this in #4077, including its advantages and disadvantages.

Changes

API changes

No public APIs were changed by this PR. However, a new class LevelDBPlayerDataProvider has been added to the pocketmine\player namespace.

Behavioural changes

  • player.default-data-format is added to pocketmine.yml, with options leveldb and datfile.
  • leveldb is now used by default for new servers. Servers with existing data will continue to automatically use the old datfile storage provider unless they delete their pocketmine.yml or add the new player.default-data-format option to their pocketmine.yml.

Backwards compatibility

No backwards compatibility issues currently known.

Follow-up

Convert data from the datfile storage system to leveldb. This would require some kind of format detection to enable this.

Tests

Tested locally using both datfile and leveldb configuration options. Everything seems to work as expected.

This does NOT convert older stored data - just allows storing data in LevelDB.

This changes the default format used by new servers, but existing servers will continue to use the old datfile storage system for now.
@dktapps dktapps added Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP Category: UI Related to the user interface (e.g. commands, terminal output) labels Dec 23, 2022
@IvanCraft623
Copy link
Member

No migrator?

@dktapps
Copy link
Member Author

dktapps commented Dec 24, 2022

See follow-up section.

Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 2, 2024
@dktapps
Copy link
Member Author

dktapps commented Dec 1, 2024

Main problem with this PR is that it's not possible to delete a specific player's data. The PlayerDataProvider interface would need to be changed to permit this. This isn't a big deal since the interface is de facto internal right now anyway (considering there's no way to provide a custom provider to the Server right now anyway), but this should be addressed before merging the PR.

@dktapps dktapps added the Status: Blocked Depends on other changes which are yet to be completed label Dec 1, 2024
@dktapps dktapps requested a review from a team as a code owner December 1, 2024 20:31
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 1, 2024
this is restricted to console to match the access level previously needed to delete player.dat files.
@dktapps
Copy link
Member Author

dktapps commented Dec 25, 2024

Closing this as I don't think it's worth the migration effort.

Between key ordering, and storing values in the same place as keys (requiring rewriting of values on DB sort), LevelDB is really poorly suited to any form of blob data storage, as we've seen with the performance issues of large worlds.

Considering that player data total size could easily reach multiple GB on highly active servers, this change would likely cause paying a large compaction performance hit. Again, this would be for data ordering which we don't even want anyway.

It may still be worth keeping a LevelDB in the playerdata to allow mapping player identifiers to the files their data are stored in, e.g. LevelDB containing maps of:

  • Player name -> .dat file name
  • UUID -> .dat name
  • XUID -> .dat name

This would allow solving some old issues with player data identification.

However, putting the data in LevelDB directly is likely to cause performance degradation for large servers.

An alternative like RocksDB's BlobDB would be much better suited to a task like this.

@dktapps dktapps closed this Dec 25, 2024
@dktapps dktapps deleted the leveldb-player-data branch December 25, 2024 14:01
@dktapps dktapps added Resolution: Declined PR has been declined by maintainers and removed Status: Blocked Depends on other changes which are yet to be completed labels Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Category: UI Related to the user interface (e.g. commands, terminal output) Performance Resolution: Declined PR has been declined by maintainers Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
Status: Abandoned
Development

Successfully merging this pull request may close these issues.

2 participants