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

Storage rewrite - Phase 1 #4065

Merged
merged 14 commits into from
Jan 2, 2024
1 change: 1 addition & 0 deletions .adr-dir
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
docs/adr
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
/.settings/
/.idea/
/.vscode/
/data-store/

dependency-reduced-pom.xml

Expand Down
120 changes: 120 additions & 0 deletions docs/adr/0001-storage-layer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# 1. Storage layer

Date: 2023-11-15

**DO NOT rely on any APIs introduced until we finish the work completely!**

## Status

Proposed

## Context

Slimefun has been around for a very long time and due to that, the way we
wrote persistence of data has also been around for a very long time.
While Slimefun has grown, the storage layer has never been adapted.
This means that even all these years later, it's using the same old saving/loading.
This isn't necessarily always bad, however, as Slimefun has grown both in terms of content
and the servers using it - we've seen some issues.

Today, files are saved as YAML files (sometimes with just a JSON object per line),
which is good for a config format but not good for a data store. It can create very large files
that can get corrupted, the way we've been saving data often means loading it all at once as well
rather than lazy-loading and generally isn't very performant.

For a long time we've been talking about rewriting our data storage in multiple forms
(you may have seen this referenced for "BlockStorage rewrite" or "SQL for PlayerProfiles", etc.).
Now is the time we start to do this, this will be a very large change and will not be done quickly or rushed.

This ADR talks about the future of our data persistence.

## Decision

We want to create a new storage layer abstraction and implementations
which will be backwards-compatible but open up new ways of storing data
within Slimefun. The end end goal is we can quickly and easily support
new storage backends (such as binary storage, SQL, etc.) for things like
[PlayerProfile](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java), [BlockStorage](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/me/mrCookieSlime/Slimefun/api/BlockStorage.java), etc.

We also want to be generally more efficient in the way we save and load data.
Today, we load way more than is required.
We can improve memory usage by only loading what we need, when we need it.

We will do this incrementally and at first, in an experimental context.
In that regard, we should aim to minimise the blast radius and lift as much
as possible.

### Quick changes overview

* New abstraction over storage to easily support multiple backends.
* Work towards moving away from the legacy YAML based storage.
* Lazy load and save data to more efficiently handle the data life cycle.

### Implementation details

There is a new interface called [`Storage`]() which is what all storage
backends will implement.
This will have methods for loading and saving things like
[`PlayerProfile`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java) and [`BlockStorage`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/me/mrCookieSlime/Slimefun/api/BlockStorage.java).

Then, backends will implement these
(e.g. [`LegacyStorageBackend`](TBD) (today's YAML situation))
in order to support these functions.
Not all storage backends are required support each data type.
e.g. SQL may not support [`BlockStorage`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/me/mrCookieSlime/Slimefun/api/BlockStorage.java).


## Addons

The goal is that Addons will be able to use and implement new storage backends
if they wish and also be extended so they can load/save things as they wish.

The first few iterations will not focus on Addon support. We want to ensure
this new storage layer will work and supports what we need it to today.

This ADR will be updated when we get to supporting Addons properly.

## Considerations

This will be a big change therefore we will be doing it as incrementally as
possible.
Changes will be tested while in the PR stage and merged into the Dev releases when possible.
We may do an experimental release if required.

The current plan looks like this:

* Phase 1 - Implement legacy data backend for [`PlayerProfile`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java).
* We want to load player data using the new storage layer with the current
data system.
* We'll want to monitor for any possible issues and generally refine
how this system should look
* Phase 2 - Implement new experimental binary backend for [`PlayerProfile`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java).
* Create a new backend for binary storage
* Implement in an experimental capacity and allow users to opt-in
* Provide a warning that this is **experimental** and there will be bugs.
* Implement new metric for storage backend being used
* Phase 3 - Mark the new backend as stable for [`PlayerProfile`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/io/github/thebusybiscuit/slimefun4/api/player/PlayerProfile.java).
* Mark it as stable and remove the warnings once we're sure things are
working correctly
* Create a migration path for users currently using "legacy".
* **MAYBE** enable by default for new servers?
* Phase 4 - Move [`BlockStorage`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/me/mrCookieSlime/Slimefun/api/BlockStorage.java) to new storage layer.
* The big one! We're gonna tackle adding this to BlockStorage.
This will probably be a large change and we'll want to be as
careful as possible here.
* Implement `legacy` and `binary` as experimental storage backends
for BlockStorage and allow users to opt-in
* Provide a warning that this is **experimental** and there will be bugs.
* Phase 5 - Mark the new storage layer as stable for [`BlockStorage`](https://github.com/Slimefun/Slimefun4/blob/bbfb9734b9f549d7e82291eff041f9b666a61b63/src/main/java/me/mrCookieSlime/Slimefun/api/BlockStorage.java).
* Mark it as stable and remove the warnings once we're sure things are
working correctly
* Ensure migration path works here too.
* **MAYBE** enable by default for new servers?
* Phase 6 - Finish up and move anything else we want over
* Move over any other data stores we have to the new layer
* We should probably still do experimental -> stable but it should have
less of a lead time.

## State of work

* Phase 1: In progress
11 changes: 11 additions & 0 deletions docs/adr/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# ADR

An ADR (Architecture Decision Record) is a document describing large changes, why we made them, etc.

## Making a new ADR

If you're making a large change to Slimefun, we recommend creating an ADR
in order to document why this is being made and how it works for future contributors.

Please follow the general format of the former ADRs or use a tool
such as [`adr-tools`](https://github.com/npryce/adr-tools) to generate a new document.
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ public void addWaypoint(@Nonnull Player p, @Nonnull String name, @Nonnull Locati
}
}

profile.addWaypoint(new Waypoint(profile, id, event.getLocation(), event.getName()));
profile.addWaypoint(new Waypoint(p.getUniqueId(), id, event.getLocation(), event.getName()));

SoundEffect.GPS_NETWORK_ADD_WAYPOINT.playFor(p);
Slimefun.getLocalization().sendMessage(p, "gps.waypoint.added", true);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package io.github.thebusybiscuit.slimefun4.api.gps;

import java.util.Objects;
import java.util.UUID;

import javax.annotation.Nonnull;
import javax.annotation.ParametersAreNonnullByDefault;

import org.apache.commons.lang.Validate;
import org.bukkit.Bukkit;
import org.bukkit.Location;
import org.bukkit.World.Environment;
import org.bukkit.entity.Player;
Expand All @@ -30,16 +32,16 @@
*/
public class Waypoint {

private final PlayerProfile profile;
private final UUID ownerId;
private final String id;
private final String name;
private final Location location;

/**
* This constructs a new {@link Waypoint} object.
*
* @param profile
* The owning {@link PlayerProfile}
* @param ownerId
* The owning {@link Player}'s {@link UUID}
* @param id
* The unique id for this {@link Waypoint}
* @param loc
Expand All @@ -48,26 +50,40 @@ public class Waypoint {
* The name of this {@link Waypoint}
*/
@ParametersAreNonnullByDefault
public Waypoint(PlayerProfile profile, String id, Location loc, String name) {
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
Validate.notNull(profile, "Profile must never be null!");
public Waypoint(UUID ownerId, String id, Location loc, String name) {
Validate.notNull(ownerId, "owner ID must never be null!");
Validate.notNull(id, "id must never be null!");
Validate.notNull(loc, "Location must never be null!");
Validate.notNull(name, "Name must never be null!");

this.profile = profile;
this.ownerId = ownerId;
this.id = id;
this.location = loc;
this.name = name;
}

/**
* This returns the owner of the {@link Waypoint}.
* This returns the owner {@link UUID} of the {@link Waypoint}.
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
*
* @return The corresponding owner {@link UUID}
*/
@Nonnull
public UUID getOwnerId() {
return this.ownerId;
}

/**
* This returns the owner of the {@link Waypoint}.
*
* @return The corresponding {@link PlayerProfile}
*
* @deprecated Use {@link #getUuid()} instead
*/
@Nonnull
@Deprecated
public PlayerProfile getOwner() {
return profile;
// This is jank and should never actually return null
return PlayerProfile.find(Bukkit.getOfflinePlayer(ownerId)).orElse(null);
}

/**
Expand Down Expand Up @@ -126,7 +142,7 @@ public ItemStack getIcon() {
*/
@Override
public int hashCode() {
return Objects.hash(profile.getUUID(), id, name, location);
return Objects.hash(this.ownerId, this.id, this.name, this.location);
}

/**
Expand All @@ -139,7 +155,9 @@ public boolean equals(Object obj) {
}

Waypoint waypoint = (Waypoint) obj;
return profile.getUUID().equals(waypoint.getOwner().getUUID()) && id.equals(waypoint.getId()) && location.equals(waypoint.getLocation()) && name.equals(waypoint.getName());
return this.ownerId.equals(waypoint.getOwnerId())
&& id.equals(waypoint.getId())
&& location.equals(waypoint.getLocation())
&& name.equals(waypoint.getName());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

import java.io.File;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;

import javax.annotation.Nonnull;
import javax.annotation.ParametersAreNonnullByDefault;

import org.bukkit.Bukkit;
import org.bukkit.entity.HumanEntity;
Expand Down Expand Up @@ -34,13 +37,22 @@ public class PlayerBackpack {

private static final String CONFIG_PREFIX = "backpacks.";

private final PlayerProfile profile;
private final UUID ownerId;
private final int id;
private final Config cfg;

@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
private PlayerProfile profile;
@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
private Config cfg;
private Inventory inventory;
private int size;

private PlayerBackpack(@Nonnull UUID ownerId, int id, int size) {
this.ownerId = ownerId;
this.id = id;
this.size = size;
}

/**
* This constructor loads an existing Backpack
*
Expand All @@ -49,6 +61,7 @@ public class PlayerBackpack {
* @param id
* The id of this Backpack
*/
@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
public PlayerBackpack(@Nonnull PlayerProfile profile, int id) {
this(profile, id, profile.getConfig().getInt(CONFIG_PREFIX + id + ".size"));

Expand All @@ -67,11 +80,13 @@ public PlayerBackpack(@Nonnull PlayerProfile profile, int id) {
* @param size
* The size of this Backpack
*/
@Deprecated
public PlayerBackpack(@Nonnull PlayerProfile profile, int id, int size) {
if (size < 9 || size > 54 || size % 9 != 0) {
throw new IllegalArgumentException("Invalid size! Size must be one of: [9, 18, 27, 36, 45, 54]");
}

this.ownerId = profile.getUUID();
this.profile = profile;
this.id = id;
this.cfg = profile.getConfig();
Expand All @@ -97,9 +112,14 @@ public int getId() {
*
* @return The owning {@link PlayerProfile}
*/
@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
@Nonnull
public PlayerProfile getOwner() {
return profile;
return profile != null ? profile : PlayerProfile.find(Bukkit.getOfflinePlayer(ownerId)).orElse(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same but not sure how else to keep compatibility here haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

public UUID getOwnerId() {
return this.ownerId;
}

/**
Expand Down Expand Up @@ -172,7 +192,6 @@ public void setSize(int size) {
}

this.size = size;
cfg.setValue(CONFIG_PREFIX + id + ".size", size);

Inventory inv = Bukkit.createInventory(null, size, "Backpack [" + size + " Slots]");

Expand All @@ -188,6 +207,7 @@ public void setSize(int size) {
/**
* This method will save the contents of this backpack to a {@link File}.
*/
@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
public void save() {
for (int i = 0; i < size; i++) {
cfg.setValue(CONFIG_PREFIX + id + ".contents." + i, inventory.getItem(i));
Expand All @@ -198,8 +218,43 @@ public void save() {
* This method marks the backpack dirty, it will then be queued for an autosave
* using {@link PlayerBackpack#save()}
*/
@Deprecated
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
public void markDirty() {
WalshyDev marked this conversation as resolved.
Show resolved Hide resolved
profile.markDirty();
if (profile != null) {
profile.markDirty();
}
}

private void setContents(int size, HashMap<Integer, ItemStack> contents) {
if (this.inventory == null) {
this.inventory = Bukkit.createInventory(null, size, "Backpack [" + size + " Slots]");
}

for (int i = 0; i < size; i++) {
this.inventory.setItem(i, contents.get(i));
}
}

// NTS: The data loading is handled in the storage layer but we handle this specific class here
@ParametersAreNonnullByDefault
public static PlayerBackpack load(UUID ownerId, int id, int size, HashMap<Integer, ItemStack> contents) {
PlayerBackpack backpack = new PlayerBackpack(ownerId, id, size);

backpack.setContents(size, contents);

return backpack;
}

@ParametersAreNonnullByDefault
public static PlayerBackpack newBackpack(UUID ownerId, int id, int size) {
if (size < 9 || size > 54 || size % 9 != 0) {
throw new IllegalArgumentException("Invalid size! Size must be one of: [9, 18, 27, 36, 45, 54]");
}

PlayerBackpack backpack = new PlayerBackpack(ownerId, id, size);

backpack.setContents(size, new HashMap<>());

return backpack;
}
}
Loading
Loading