Add sql migration and optimize plugin for big servers #56
Conversation
src/main/java/dev/unnm3d/rediseconomy/currency/CurrenciesManager.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/unnm3d/rediseconomy/currency/CurrencyMigration.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/unnm3d/rediseconomy/currency/migration/OfflinePlayerCurrencyMigration.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public String getProvider() { | ||
| return "SQL DATABASE"; |
There was a problem hiding this comment.
Instead of returning a economy provider name like offline player migration, the SQL migration doesn't have that
| @@ -23,6 +23,9 @@ public class Settings { | |||
| @Comment({"if true, migrates the bukkit offline uuids accounts to the default RedisEconomy currency", | |||
| "During the migration, the plugin will be disabled. Restart all RedisEconomy instances after the migration."}) | |||
| public boolean migrationEnabled = false; | |||
| @Comment({"if enabled, the plugin will migrate economy data from sql database", | |||
| "During the migration, the plugin will be disabled. Restart all RedisEconomy instances after the migration."}) | |||
| public SqlMigrateSettings sqlMigration = new SqlMigrateSettings(false, "com.mysql.cj.jdbc.Driver", "jdbc:mysql://localhost:3306/database?useSSL=false", "root", "password", "economy", "name", "uuid", "money"); | |||
There was a problem hiding this comment.
Can you please merge sqlMigration to the same data structure as migrationEnabled?
Make the record so that we'll have only a "enabled" key and a "type" key (SQL type or OfflinePlayer type)
| @@ -54,8 +54,8 @@ public CurrenciesManager(RedisManager redisManager, RedisEconomyPlugin plugin, C | |||
| this.configManager = configManager; | |||
| this.currencies = new HashMap<>(); | |||
| try { | |||
| this.nameUniqueIds = loadRedisNameUniqueIds().toCompletableFuture().get(1, TimeUnit.SECONDS); | |||
| this.lockedAccounts = loadLockedAccounts().toCompletableFuture().get(1, TimeUnit.SECONDS); | |||
| this.nameUniqueIds = loadRedisNameUniqueIds().toCompletableFuture().get(10, TimeUnit.SECONDS); | |||
There was a problem hiding this comment.
Why 10 seconds. if you have that latency it means your redis is down.
Redis isn't like MySQL, it doesn't take ages to load data.
I'm ok if you change it to 3 or 4 secs
| .append(System.getProperty("line.separator")) | ||
| )); | ||
| fw.write(sb.toString());// currency;uuid;name;balance | ||
| final Map<UUID, String> names = new HashMap<>(); |
There was a problem hiding this comment.
Do you really need to check RAM requirements?
we are talking of megabytes not gigabytes.
i tested with like 40k entries.
for 400000 entries I calculated 57MB of RAM
| for (Currency currency : currenciesManager.getCurrencies()) { | ||
| Map<UUID, Double> accounts = currency.getAccounts(); | ||
| plugin.getLogger().info("[" + currency.getCurrencyName() + "] Total accounts: " + accounts.size()); | ||
| if (accounts.isEmpty()) { |
There was a problem hiding this comment.
Ok that's fair. but check empty balances too. by default Redis will set them to %starting_balance%
| return; | ||
| } | ||
|
|
||
| plugin.langs().send(Bukkit.getConsoleSender(), plugin.langs().migrationStart.replace("%provider%", getProvider())); |
There was a problem hiding this comment.
Use plugin.getServer().getConsoleSender() please
|
|
||
| public class OfflinePlayerCurrencyMigration extends CurrencyMigration { | ||
|
|
||
| private RegisteredServiceProvider<Economy> existentProvider; |
There was a problem hiding this comment.
What's the point of using RegisteredServiceProvider?
You can directly use Vault's Economy class
|
|
||
| @Override | ||
| public String getProvider() { | ||
| return existentProvider.getProvider().getName(); |
There was a problem hiding this comment.
existentProvider.getProvider() can be null
| for (int i = 0; i < offlinePlayers.length; i++) { | ||
| final OfflinePlayer offlinePlayer = offlinePlayers[i]; | ||
| try { | ||
| double bal = existentProvider.getProvider().getBalance(offlinePlayer); |
There was a problem hiding this comment.
existentProvider.getProvider() can be null
| final List<ScoredValue<String>> balances = new ArrayList<>(); | ||
| final Map<String, String> nameUniqueIds = new HashMap<>(); | ||
|
|
||
| try (PreparedStatement count = connection.prepareStatement("SELECT COUNT(*) FROM `" + sql.table() + "`"); PreparedStatement stmt = connection.prepareStatement("SELECT ALL * FROM `" + sql.table() + "`")) { |
There was a problem hiding this comment.
Isn't it better to separate those 2 queries?
| nameUniqueIds.put(name, uuid); | ||
| updateAccountLocal(UUID.fromString(uuid), name == null ? uuid : name, money); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Send only a warning with the exception message or the user will be flooded by errors
I found multiple problems using the plugin, so I added some changes to fix them.
SQL migration
Instead of offline player migration, this concept obtain data directly from a SQL database.
Some plugins like MySQLPlayerDataBridge did not allow me to migrate the data because it did not exist for some players when using Vault API.
Big servers data handling
For example, one of my server modes have more than 350.000 accounts, so I extended the name loading timeout and also created an optimized backup command.
Previously, using
/backup-economythe plugin took 45 minutes to save accounts into file, now it took no more than 5 seconds.