From f5a13122e91f4dec7112375241529cf21d72f290 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Le=C3=9Fmann?= Date: Thu, 14 Nov 2019 16:21:40 +0100 Subject: [PATCH 1/5] Improve user-service performance by caching results for 30 seconds --- build.gradle | 2 +- .../service/teamspeak/CachedUserResult.java | 26 ++++++++++++ .../jeak/service/teamspeak/UserService.java | 41 +++++++++++++++++-- 3 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 src/main/java/de/fearnixx/jeak/service/teamspeak/CachedUserResult.java diff --git a/build.gradle b/build.gradle index 7e3868d4..c162c28f 100644 --- a/build.gradle +++ b/build.gradle @@ -1,5 +1,5 @@ group 'de.fearnixx' -version '1.0.3' +version '1.0.4' def projectVersion = project.version apply plugin: 'java' diff --git a/src/main/java/de/fearnixx/jeak/service/teamspeak/CachedUserResult.java b/src/main/java/de/fearnixx/jeak/service/teamspeak/CachedUserResult.java new file mode 100644 index 00000000..2faa8658 --- /dev/null +++ b/src/main/java/de/fearnixx/jeak/service/teamspeak/CachedUserResult.java @@ -0,0 +1,26 @@ +package de.fearnixx.jeak.service.teamspeak; + +import de.fearnixx.jeak.teamspeak.data.IUser; + +import java.time.LocalDateTime; +import java.util.ArrayList; +import java.util.List; + +public class CachedUserResult { + + private final List users; + private final LocalDateTime expiry; + + public CachedUserResult(List users, LocalDateTime expiry) { + this.users = users; + this.expiry = expiry; + } + + public List getUsers() { + return new ArrayList<>(users); + } + + public LocalDateTime getExpiry() { + return expiry; + } +} diff --git a/src/main/java/de/fearnixx/jeak/service/teamspeak/UserService.java b/src/main/java/de/fearnixx/jeak/service/teamspeak/UserService.java index 6dc2991f..5603945f 100644 --- a/src/main/java/de/fearnixx/jeak/service/teamspeak/UserService.java +++ b/src/main/java/de/fearnixx/jeak/service/teamspeak/UserService.java @@ -1,5 +1,6 @@ package de.fearnixx.jeak.service.teamspeak; +import de.fearnixx.jeak.Main; import de.fearnixx.jeak.event.bot.IBotStateEvent; import de.fearnixx.jeak.reflect.FrameworkService; import de.fearnixx.jeak.reflect.IInjectionService; @@ -13,8 +14,12 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.time.LocalDateTime; import java.util.List; import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Predicate; +import java.util.function.Supplier; /** * Small loader around the user service that decides whether or not to use direct DB access or queries. @@ -23,6 +28,7 @@ public class UserService implements IUserService { public static final String PERSISTENCE_UNIT_NAME = "teamspeak"; + private static final int USR_CACHE_TTL = Main.getProperty("jeak.cache.keepUsersSecs", 30); private static final Logger logger = LoggerFactory.getLogger(UserService.class); @Inject @@ -34,6 +40,8 @@ public class UserService implements IUserService { @Inject private IInjectionService injectionService; + private final List userCache = new CopyOnWriteArrayList<>(); + private IUserService serviceImplementation; @Listener @@ -54,17 +62,44 @@ public void onPreInitialize(IBotStateEvent.IPreInitializeEvent event) { @Override public List findUserByUniqueID(String ts3uniqueID) { - return serviceImplementation.findUserByUniqueID(ts3uniqueID); + return computeIfNotCached( + u -> u.getClientUniqueID().equals(ts3uniqueID), + () -> serviceImplementation.findUserByUniqueID(ts3uniqueID) + ); + } + + private List computeIfNotCached(Predicate matchBy, Supplier> getBy) { + synchronized (userCache) { + final LocalDateTime now = LocalDateTime.now(); + userCache.removeIf(entry -> entry.getExpiry().isBefore(now)); + Optional optResult = userCache.stream() + .filter(entry -> entry.getUsers().stream().allMatch(matchBy)) + .findFirst(); + return optResult.map(CachedUserResult::getUsers) + .orElseGet(() -> { + List users = getBy.get(); + CachedUserResult result = + new CachedUserResult(users, LocalDateTime.now().plusSeconds(USR_CACHE_TTL)); + userCache.add(result); + return result.getUsers(); + }); + } } @Override public List findUserByDBID(int ts3dbID) { - return serviceImplementation.findUserByDBID(ts3dbID); + return computeIfNotCached( + u -> u.getClientDBID().equals(ts3dbID), + () -> serviceImplementation.findUserByDBID(ts3dbID) + ); } @Override public List findUserByNickname(String ts3nickname) { - return serviceImplementation.findUserByNickname(ts3nickname); + return computeIfNotCached( + u -> u.getNickName().contains(ts3nickname), + () -> serviceImplementation.findUserByNickname(ts3nickname) + ); } @Override From 954b21995a8f4b73642f3fea338436220a0ab399 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Le=C3=9Fmann?= Date: Thu, 14 Nov 2019 18:23:12 +0100 Subject: [PATCH 2/5] Fix a bug that caused dataSourceOpts not to be applied to Hikari --- .../service/database/HHPersistenceUnit.java | 48 +++++++++++++++++-- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java index 37c3e8a8..3ac320e8 100644 --- a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java +++ b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java @@ -19,16 +19,23 @@ import javax.persistence.PersistenceException; import javax.sql.DataSource; import java.io.IOException; -import java.util.HashMap; -import java.util.LinkedList; -import java.util.List; -import java.util.Map; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.*; public class HHPersistenceUnit extends Configurable implements IPersistenceUnit, AutoCloseable { private static final Logger logger = LoggerFactory.getLogger(HHPersistenceUnit.class); private static final String DEFAULT_DATASOURCE_CONFIG = "/database/defaultDS.json"; + private static final Map HIKARI_CONF_SETTER = new HashMap<>(); + + static { + Arrays.stream(HikariConfig.class.getDeclaredMethods()) + .filter(m -> m.getName().startsWith("set")) + .forEach(m -> HIKARI_CONF_SETTER.put(m.getName(), m)); + } + private final Map dataSourceOpts = new HashMap<>(); private final List entityManagers = new LinkedList<>(); private final IConfig config; @@ -109,8 +116,8 @@ private boolean initializeHikariSource() { hikariConfig.setUsername(getUsername()); hikariConfig.setPassword(getPassword()); - dataSourceOpts.forEach(hikariConfig::addDataSourceProperty); try { + dataSourceOpts.forEach((key, value) -> hardSetDSProperty(key, value, hikariConfig)); hikariDS = new HikariDataSource(hikariConfig); return true; } catch (HikariPool.PoolInitializationException e) { @@ -119,6 +126,37 @@ private boolean initializeHikariSource() { } } + private void hardSetDSProperty(String key, String value, HikariConfig hikariConfig) { + String methodName = "set" + key.substring(0, 1).toUpperCase() + key.substring(1); + Method setter = HIKARI_CONF_SETTER.getOrDefault(methodName, null); + if (setter == null) { + NoSuchMethodException iA = new NoSuchMethodException("Hikari does not support setting the dataSource property: " + key); + throw new HikariPool.PoolInitializationException(iA); + } + + try { + Class paramType = setter.getParameterTypes()[0]; + Object param = null; + + if (paramType.isAssignableFrom(Integer.class) || paramType.equals(int.class)) { + param = Integer.parseInt(value); + } else if (paramType.isAssignableFrom(Long.class) || paramType.equals(long.class)) { + param = Long.parseLong(value); + } else if (paramType.isAssignableFrom(String.class)) { + param = value; + } else if (paramType.isAssignableFrom(Boolean.class) || paramType.equals(boolean.class)) { + param = "true".equalsIgnoreCase(value) || "1".equals(value); + } + + logger.info("Setting bean property \"{}\" -> \"{}\" on HikariConfig of \"{}\"", key, value, unitId); + setter.invoke(hikariConfig, param); + hikariConfig.addDataSourceProperty(key, value); + } catch (IllegalAccessException | InvocationTargetException | NumberFormatException e) { + Throwable cause = e instanceof InvocationTargetException ? e.getCause() : e; + throw new HikariPool.PoolInitializationException(cause); + } + } + private boolean initializeHibernateServices() { try { StandardServiceRegistryBuilder registryBuilder = new StandardServiceRegistryBuilder(baseRegistry); From ec72c8b21d0dbdc840be5fb1faf42b2fd4d27223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Le=C3=9Fmann?= Date: Thu, 14 Nov 2019 18:31:14 +0100 Subject: [PATCH 3/5] Throw an error when DS options are not strings --- .../de/fearnixx/jeak/service/database/HHPersistenceUnit.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java index 3ac320e8..c4e65ed6 100644 --- a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java +++ b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java @@ -106,8 +106,7 @@ private void readConfiguration() { .optMap() .ifPresent(map -> map.forEach((key, value) -> - value.optString().ifPresent(strVal -> - dataSourceOpts.put(key, strVal)))); + dataSourceOpts.put(key, value.asString()))); } private boolean initializeHikariSource() { From 9e36b8d80d4c390f71205a64e61e7da102fe72be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Le=C3=9Fmann?= Date: Thu, 14 Nov 2019 18:37:59 +0100 Subject: [PATCH 4/5] Fix db-based permission service not returning its connections to pool --- .../AbstractTS3PermissionProvider.java | 5 -- .../teamspeak/DBPermissionProvider.java | 48 ++++++++++--------- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/AbstractTS3PermissionProvider.java b/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/AbstractTS3PermissionProvider.java index c35f5d7a..b791f4da 100644 --- a/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/AbstractTS3PermissionProvider.java +++ b/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/AbstractTS3PermissionProvider.java @@ -17,8 +17,6 @@ public abstract class AbstractTS3PermissionProvider implements ITS3PermissionProvider { - private static final Logger logger = LoggerFactory.getLogger(AbstractTS3PermissionProvider.class); - private final PermIdCache permIdCache = new PermIdCache(); @Inject @@ -41,9 +39,6 @@ protected IServer getServer() { return server; } - @Override - public abstract void clearCache(ITS3Permission.PriorityType type, Integer optClientOrGroupID, Integer optChannelID); - @Override public Optional getActivePermission(String permSID, String clientTS3UniqueID) { Optional optClient = userService.findClientByUniqueID(clientTS3UniqueID) diff --git a/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/DBPermissionProvider.java b/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/DBPermissionProvider.java index 153b54c3..b3f1f1ff 100644 --- a/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/DBPermissionProvider.java +++ b/src/main/java/de/fearnixx/jeak/service/permission/teamspeak/DBPermissionProvider.java @@ -7,6 +7,7 @@ import org.slf4j.LoggerFactory; import javax.persistence.PersistenceUnit; +import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; @@ -57,30 +58,31 @@ private Optional selectFromTable(Integer idOne, String permSID, sql += " AND id2 = ?"; } - try (PreparedStatement statement = persistenceUnit.getDataSource() - .getConnection() - .prepareStatement(sql)) { - statement.setInt(1, getServerId()); - statement.setString(2, permSID); - statement.setInt(3, idOne); - if (idTwo != null) { - statement.setInt(4, idTwo); + try (Connection dbConnection = persistenceUnit.getDataSource().getConnection()) { + try (PreparedStatement statement = dbConnection + .prepareStatement(sql)) { + statement.setInt(1, getServerId()); + statement.setString(2, permSID); + statement.setInt(3, idOne); + if (idTwo != null) { + statement.setInt(4, idTwo); + } + + TS3Permission perm; + try (ResultSet res = statement.executeQuery()) { + perm = new TS3Permission(ITS3Permission.PriorityType.CLIENT, permSID); + if (res.next()) { + perm.setValue(res.getInt(1)); + perm.setNegated(res.getInt(2) == 1); + perm.setSkipped(res.getInt(3) == 1); + } else { + perm.setValue(0); + perm.setNegated(false); + perm.setSkipped(false); + } + } + return Optional.of(perm); } - - ResultSet res = statement.executeQuery(); - TS3Permission perm = new TS3Permission(ITS3Permission.PriorityType.CLIENT, permSID); - if (res.isBeforeFirst()) { - res.next(); - perm.setValue(res.getInt(1)); - perm.setNegated(res.getInt(2) == 1); - perm.setSkipped(res.getInt(3) == 1); - } else { - perm.setValue(0); - perm.setNegated(false); - perm.setSkipped(false); - } - return Optional.of(perm); - } catch (SQLException e) { logger.error("Failed to get permission value from database! {} for {}", permSID, idOne, e); return Optional.empty(); From 60ad8ed4966797ed121e49d15dbca00f6d6e37d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Le=C3=9Fmann?= Date: Thu, 14 Nov 2019 18:40:38 +0100 Subject: [PATCH 5/5] Issue a more readable warning --- .../jeak/service/database/HHPersistenceUnit.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java index c4e65ed6..ccc4c2f2 100644 --- a/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java +++ b/src/main/java/de/fearnixx/jeak/service/database/HHPersistenceUnit.java @@ -105,8 +105,14 @@ private void readConfiguration() { getConfig().getNode("dataSourceOpts") .optMap() .ifPresent(map -> - map.forEach((key, value) -> - dataSourceOpts.put(key, value.asString()))); + map.forEach((key, value) -> { + Optional optVal = value.optString(); + if (optVal.isEmpty()) { + logger.error("Cannot set DS property that is not a string! {} => {}", unitId, key); + } else { + dataSourceOpts.put(key, value.asString()); + } + })); } private boolean initializeHikariSource() {