Storage rewrite - Phase 1 #4065

WalshyDev · 2023-12-19T14:44:09Z

Description

Ok... bear with me on this!

This PR is the start of our new storage layer. Famously known as the "BlockStorage rewrite" or "SQL for player data".
This PR goes over the goals, and how we get there and starts implementation of phase 1.

Please read through the whole ADR as that describes the goal that I'm going for here.
Please review and comment on the ADR as well as the code being done here.

This PR is mainly about kicking things off, sharing the direction/goal and starting the process. It is not about implementing the API, the additional backends or anything else. The API seen in this PR is a bare minimum and will change to be more inline with what we will probably go for going forward.

Proposed changes

Save and load the PlayerProfile with the new storage layer
- Move saving/loading outside of the PlayerProfile class as much as possible
Move away from passing around PlayerProfile into things
Start to implement a new storage API (this is not final and will drastically change)
Moved data out of PlayerProfile to make it easier to handle -- see open questions
Remove dirty from PlayerProfile -- see open questions

Related Issues (if applicable)

N/A

Checklist

I have fully tested the proposed changes and promise that they will not break everything into chaos.
I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
I followed the existing code standards and didn't mess up the formatting.
I did my best to add documentation to any public classes or methods I added.
I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
I added sufficient Unit Tests to cover my code.

Open questions

Where should PlayerProfile data live?
- Within this PR, it was moved to a PlayerData class as it was much easier to manage. PlayerProfile does a lot. Does this make sense? Do we want it back in PlayerProfile?
Do we care about tracking dirty state?
- Generally, does this make sense if we save/load efficiently and always off-main-thread? The PlayerProfile was always marked dirty on the first save anyway but would not be re-saved after (unless someone did something like open a backpack).
Backpacks are currently lazy-loaded but this moves it away from lazy-loading. Is this ok?
- I don't think backpacks make up much memory in general so I think this is fine. It's more of a lift and complication to lazy-load just backpacks. Something that can be done but probably a question for down the road.
Does the direction of the ADR make sense? Agree with the steps?

Testing

This PR focuses on the PlayerProfile and associated data. Therefore, we'd like testing on:

Do researches work still? If you research something and restart, is it still researched? Can you keep researching things? Is anything broken there?
Do backpacks work still? If you create a backpack with items and restart, does it still look good? Is anything broken there?
Do waypoints work still? If you create a waypoint and restart, does it still look good? Is anything broken there? Player specific and shared (not sure how shared work -- I suspect those are broken)

github-actions · 2023-12-19T14:47:53Z

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: de9dd204

https://preview-builds.walshy.dev/download/Slimefun/4065/de9dd204

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

Sfiguz7

Haven't reviewed all, will continue in the next days. Still left some comments on what I went over

docs/adr/0001-storage-layer.md

Sfiguz7 · 2023-12-19T20:54:04Z

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

    @Nonnull
    public PlayerProfile getOwner() {
-        return profile;
+        return profile != null ? profile : PlayerProfile.find(Bukkit.getOfflinePlayer(ownerId)).orElse(null);


This is jank and I know, but seeing orElse(null) in a @Nonnull method makes my eyes bleed

same but not sure how else to keep compatibility here haha

Maybe orElseThrow to keep the nonnull contract and give some info in an error if this somehow fails?

src/main/java/io/github/thebusybiscuit/slimefun4/storage/backend/legacy/LegacyStorage.java

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java

Sfiguz7 · 2023-12-19T21:09:25Z

src/main/java/io/github/thebusybiscuit/slimefun4/storage/backend/legacy/LegacyStorage.java

+        Config playerFile = new Config("data-storage/Slimefun/Players/" + uuid + ".yml");
+        // Not too sure why this is it's own file
+        Config waypointsFile = new Config("data-storage/Slimefun/waypoints/" + uuid + ".yml");


Should we extract these and make them class variables? They are used in both load and save, and the first one is still referenced in PlayerProfile.java just for a getter that I see, and who knows if they are elsewhere that I have yet to see or can't remember. Sounds more robust to just have them be in one place, or is there a reason to keep them elsewhere too?

Bit awkward since they rely on a UUID. I could put the string as a global and format but it's only used twice in here so wasn't the biggest fan of that.
This API isn't great and not really expandable so I plan to work on figuring out how that should look post this PR. Once that is done, we may be able to globalise in some way. Not sure how yet.

src/main/java/io/github/thebusybiscuit/slimefun4/storage/backend/legacy/LegacyStorage.java

src/main/java/io/github/thebusybiscuit/slimefun4/api/gps/Waypoint.java

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/Slimefun.java

src/main/java/io/github/thebusybiscuit/slimefun4/storage/data/PlayerData.java

J3fftw1 · 2023-12-20T21:06:16Z

Where should PlayerProfile data live?

Within this PR, it was moved to a PlayerData class as it was much easier to manage. PlayerProfile does a lot. Does this make sense? Do we want it back in PlayerProfile?

If its easier to manage we should keep it out of PlayerProfile. No reason to make our own lifes harder

Do we care about tracking dirty state?

Generally, does this make sense if we save/load efficiently and always off-main-thread? The PlayerProfile was always marked dirty on the first save anyway but would not be re-saved after (unless someone did something like open a backpack).

If its handled correctly no if its not yes.
So no since we are gonna handle it correctly :p

Backpacks are currently lazy-loaded but this moves it away from lazy-loading. Is this ok?

I don't think backpacks make up much memory in general so I think this is fine. It's more of a lift and complication to lazy-load just backpacks. Something that can be done but probably a question for down the road.

We might need some examples of servers with mass use of backpacks to answer this.
If the backpacks are big in size we should move away from lazy-loading.

Does the direction of the ADR make sense? Agree with the steps?

On phase 3 we mark the new backend as stable for playerprofile. then the quesiton should we enable it by default for the servers is yes this shouldnt be a maybe otherwise we arent going into phase 3.
Same actually for all other phases.
We cant go to the next phase when we dont know its stable.
The maybe should be removed.

I agree otherwise with the steps.
I think we should lay some grounds on testing. like what do we need to be tested. maybe write up a doc on what we want to be tested.

General question.
Once we are within phases do we want to "change freeze" other PR's cuz of this or do we just keep continuing to work since we dont know the pace of the work being done since this isnt just you doing the job.

WalshyDev · 2023-12-21T05:25:46Z

Once we are within phases do we want to "change freeze" other PR's cuz of this or do we just keep continuing to work since we dont know the pace of the work being done since this isnt just you doing the job.

Imo, this is a question we'll ask ourselves on a case by case basis. I think for this one, not needed. BS phase, maybe since it's so massive. This one should be easy to spot the bugs coming from this.

J3fftw1

LGTM i think we should move to phase 2

Seggan · 2023-12-27T21:09:25Z

Agree

Sfiguz7

Some painful things but discussed in DMs and they're fine. LGTM

J3fftw1 · 2023-12-31T08:43:19Z

Walked through everything again and it seems to work.

JustAHuman-xD

Not sure if what I mentioned should be "gating" changes so just doing comment

src/main/java/io/github/thebusybiscuit/slimefun4/api/gps/Waypoint.java

src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerBackpack.java

src/main/java/io/github/thebusybiscuit/slimefun4/storage/data/PlayerData.java

sonarqubecloud · 2024-01-02T02:05:34Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

34 New issues
0 Security Hotspots
74.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

JustAHuman-xD

LGTM, if I missed anything it would be on the tests but I think it's good 😅

WalshyDev added 8 commits December 16, 2023 04:45

Phase 1 - wip

21ce6c4

Add some tests

a15faf0

wip

5af81f1

Save waypoints

fc7bc93

Implement backpacks

71d52af

Add tests for waypoints

7ac9aab

Changes to ADR

0d103a9

Small changes

5c14b5f

WalshyDev requested a review from a team as a code owner December 19, 2023 14:44

Slimefun deleted a comment from github-actions bot Dec 19, 2023

WalshyDev added the 🔧 API This Pull Request introduces API changes. label Dec 19, 2023

WalshyDev removed the ✨ Fix This Pull Request fixes an issue. label Dec 19, 2023

Sfiguz7 reviewed Dec 19, 2023

View reviewed changes

Fix englandish and some small changes to UUID in PlayerProfile

b13392a

J3fftw1 suggested changes Dec 20, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

WalshyDev added 3 commits December 20, 2023 21:09

Fix saving of player data in a few cases

fdbb8d1

Documentation around deprecated

d29124e

Add some more tests

6ffd63d

Some small doc updates

12a7f55

J3fftw1 previously approved these changes Dec 27, 2023

View reviewed changes

J3fftw1 self-assigned this Dec 27, 2023

WalshyDev requested review from Sefiraat and JustAHuman-xD December 27, 2023 21:10

WalshyDev assigned Sfiguz7 Dec 27, 2023

Sfiguz7 previously approved these changes Dec 27, 2023

View reviewed changes

J3fftw1 removed the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Dec 31, 2023

JustAHuman-xD reviewed Jan 1, 2024

View reviewed changes

Make old Waypoint constructor deprecated and fix javadocs

33fae27

WalshyDev dismissed stale reviews from Sfiguz7 and J3fftw1 via 33fae27 January 2, 2024 02:03

WalshyDev requested review from J3fftw1, Sfiguz7 and JustAHuman-xD January 2, 2024 02:03

JustAHuman-xD approved these changes Jan 2, 2024

View reviewed changes

J3fftw1 approved these changes Jan 2, 2024

View reviewed changes

Sfiguz7 approved these changes Jan 2, 2024

View reviewed changes

WalshyDev merged commit 4d710fa into master Jan 2, 2024
18 checks passed

WalshyDev deleted the walshy/phase-1 branch January 2, 2024 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage rewrite - Phase 1 #4065

Storage rewrite - Phase 1 #4065

WalshyDev commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading

Sfiguz7 left a comment

Sfiguz7 Dec 19, 2023

WalshyDev Dec 20, 2023

iTwins Dec 27, 2023

Sfiguz7 Dec 19, 2023

WalshyDev Dec 20, 2023

This comment was marked as resolved.

J3fftw1 commented Dec 20, 2023

WalshyDev commented Dec 21, 2023

J3fftw1 left a comment

Seggan commented Dec 27, 2023

Sfiguz7 left a comment

J3fftw1 commented Dec 31, 2023

JustAHuman-xD left a comment

sonarqubecloud bot commented Jan 2, 2024

JustAHuman-xD left a comment

Storage rewrite - Phase 1 #4065

Storage rewrite - Phase 1 #4065

Conversation

WalshyDev commented Dec 19, 2023 • edited Loading

Description

Proposed changes

Related Issues (if applicable)

Checklist

Open questions

Testing

github-actions bot commented Dec 19, 2023 • edited Loading

Slimefun preview build

Sfiguz7 left a comment

Choose a reason for hiding this comment

Sfiguz7 Dec 19, 2023

Choose a reason for hiding this comment

WalshyDev Dec 20, 2023

Choose a reason for hiding this comment

iTwins Dec 27, 2023

Choose a reason for hiding this comment

Sfiguz7 Dec 19, 2023

Choose a reason for hiding this comment

WalshyDev Dec 20, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

J3fftw1 commented Dec 20, 2023

WalshyDev commented Dec 21, 2023

J3fftw1 left a comment

Choose a reason for hiding this comment

Seggan commented Dec 27, 2023

Sfiguz7 left a comment

Choose a reason for hiding this comment

J3fftw1 commented Dec 31, 2023

JustAHuman-xD left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 2, 2024

Quality Gate passed

JustAHuman-xD left a comment

Choose a reason for hiding this comment

WalshyDev commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading