This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 0aeb42eebc Fix the race condition of reflection scanning classes
(#9167)
0aeb42eebc is described below
commit 0aeb42eebc0846611762a9ff2cdc1e1ede600727
Author: Xiaotian (Jackie) Jiang <17555551+jackie-ji...@users.noreply.github.com>
AuthorDate: Fri Aug 5 01:17:35 2022 -0700
Fix the race condition of reflection scanning classes (#9167)
---
.../broker/broker/BrokerAdminApiApplication.java | 4 +-
.../pinot/common/function/FunctionRegistry.java | 14 +---
.../api/ControllerAdminApiApplication.java | 15 ++--
.../controller/tuner/TableConfigTunerRegistry.java | 27 ++-----
.../pinot/minion/MinionAdminApiApplication.java | 4 +-
.../spi/loader/SegmentDirectoryLoaderRegistry.java | 20 ++---
.../server/starter/helix/AdminApiApplication.java | 12 ++-
.../pinot/spi/utils/PinotReflectionUtils.java | 91 +++++++++++++++++++---
.../PinotServiceManagerAdminApiApplication.java | 4 +-
9 files changed, 109 insertions(+), 82 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
index fcf151fab6..fb8c4d6f0a 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
@@ -80,9 +80,7 @@ public class BrokerAdminApiApplication extends ResourceConfig
{
} catch (IOException e) {
throw new RuntimeException("Failed to start http server", e);
}
- synchronized (PinotReflectionUtils.getReflectionLock()) {
- setupSwagger();
- }
+ PinotReflectionUtils.runWithLock(this::setupSwagger);
}
private void setupSwagger() {
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
index db8864aed7..7633e2391c 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
@@ -27,11 +27,7 @@ import java.util.Set;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
import org.apache.pinot.spi.annotations.ScalarFunction;
-import org.reflections.Reflections;
-import org.reflections.scanners.MethodAnnotationsScanner;
-import org.reflections.util.ClasspathHelper;
-import org.reflections.util.ConfigurationBuilder;
-import org.reflections.util.FilterBuilder;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -54,12 +50,8 @@ public class FunctionRegistry {
*/
static {
long startTimeMs = System.currentTimeMillis();
- Reflections reflections = new Reflections(
- new
ConfigurationBuilder().setUrls(ClasspathHelper.forPackage("org.apache.pinot"))
- .filterInputsBy(new FilterBuilder.Include(".*\\.function\\..*"))
- .setScanners(new MethodAnnotationsScanner()));
- Set<Method> methodSet =
reflections.getMethodsAnnotatedWith(ScalarFunction.class);
- for (Method method : methodSet) {
+ Set<Method> methods =
PinotReflectionUtils.getMethodsThroughReflection(".*\\.function\\..*",
ScalarFunction.class);
+ for (Method method : methods) {
if (!Modifier.isPublic(method.getModifiers())) {
continue;
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
index f7ef335ee1..d1a77d6789 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
@@ -40,12 +40,9 @@ import org.glassfish.hk2.utilities.binding.AbstractBinder;
import org.glassfish.jersey.jackson.JacksonFeature;
import org.glassfish.jersey.media.multipart.MultiPartFeature;
import org.glassfish.jersey.server.ResourceConfig;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
public class ControllerAdminApiApplication extends ResourceConfig {
- private static final Logger LOGGER =
LoggerFactory.getLogger(ControllerAdminApiApplication.class);
public static final String PINOT_CONFIGURATION = "pinotConfiguration";
private final String _controllerResourcePackages;
@@ -86,9 +83,7 @@ public class ControllerAdminApiApplication extends
ResourceConfig {
} catch (IOException e) {
throw new RuntimeException("Failed to start http server", e);
}
- synchronized (PinotReflectionUtils.getReflectionLock()) {
- setupSwagger(_httpServer);
- }
+ PinotReflectionUtils.runWithLock(this::setupSwagger);
ClassLoader classLoader =
ControllerAdminApiApplication.class.getClassLoader();
@@ -105,7 +100,7 @@ public class ControllerAdminApiApplication extends
ResourceConfig {
_httpServer.getServerConfiguration().addHttpHandler(new
CLStaticHttpHandler(classLoader, "/webapp/js/"), "/js/");
}
- private void setupSwagger(HttpServer httpServer) {
+ private void setupSwagger() {
BeanConfig beanConfig = new BeanConfig();
beanConfig.setTitle("Pinot Controller API");
beanConfig.setDescription("APIs for accessing Pinot Controller
information");
@@ -124,12 +119,12 @@ public class ControllerAdminApiApplication extends
ResourceConfig {
ClassLoader loader = this.getClass().getClassLoader();
CLStaticHttpHandler apiStaticHttpHandler = new CLStaticHttpHandler(loader,
"/api/");
// map both /api and /help to swagger docs. /api because it looks nice.
/help for backward compatibility
- httpServer.getServerConfiguration().addHttpHandler(apiStaticHttpHandler,
"/api/");
- httpServer.getServerConfiguration().addHttpHandler(apiStaticHttpHandler,
"/help/");
+ _httpServer.getServerConfiguration().addHttpHandler(apiStaticHttpHandler,
"/api/");
+ _httpServer.getServerConfiguration().addHttpHandler(apiStaticHttpHandler,
"/help/");
URL swaggerDistLocation =
loader.getResource("META-INF/resources/webjars/swagger-ui/3.23.11/");
CLStaticHttpHandler swaggerDist = new CLStaticHttpHandler(new
URLClassLoader(new URL[]{swaggerDistLocation}));
- httpServer.getServerConfiguration().addHttpHandler(swaggerDist,
"/swaggerui-dist/");
+ _httpServer.getServerConfiguration().addHttpHandler(swaggerDist,
"/swaggerui-dist/");
}
public void stop() {
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/tuner/TableConfigTunerRegistry.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/tuner/TableConfigTunerRegistry.java
index 2913e72d12..173069846b 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/tuner/TableConfigTunerRegistry.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/tuner/TableConfigTunerRegistry.java
@@ -19,25 +19,17 @@
package org.apache.pinot.controller.tuner;
import com.google.common.base.Preconditions;
-import java.net.URL;
-import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
-import org.reflections.Reflections;
-import org.reflections.scanners.ResourcesScanner;
-import org.reflections.scanners.SubTypesScanner;
-import org.reflections.scanners.TypeAnnotationsScanner;
-import org.reflections.util.ClasspathHelper;
-import org.reflections.util.ConfigurationBuilder;
-import org.reflections.util.FilterBuilder;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Helper class to dynamically register all annotated {@link Tuner} methods
+ * Helper class to dynamically register all annotated {@link Tuner} classes.
*/
public class TableConfigTunerRegistry {
private TableConfigTunerRegistry() {
@@ -63,16 +55,9 @@ public class TableConfigTunerRegistry {
}
long startTime = System.currentTimeMillis();
- List<URL> urls = new ArrayList<>();
- for (String pack : packages) {
- urls.addAll(ClasspathHelper.forPackage(pack));
- }
-
- Reflections reflections = new Reflections(
- new ConfigurationBuilder().setUrls(urls).filterInputsBy(new
FilterBuilder.Include(".*\\.tuner\\..*"))
- .setScanners(new ResourcesScanner(), new TypeAnnotationsScanner(),
new SubTypesScanner()));
- Set<Class<?>> classes = reflections.getTypesAnnotatedWith(Tuner.class);
- classes.forEach(tunerClass -> {
+ Set<Class<?>> tunerClasses =
+ PinotReflectionUtils.getClassesThroughReflection(packages,
".*\\.tuner\\..*", Tuner.class);
+ for (Class<?> tunerClass : tunerClasses) {
Tuner tunerAnnotation = tunerClass.getAnnotation(Tuner.class);
if (tunerAnnotation.enabled()) {
if (tunerAnnotation.name().isEmpty()) {
@@ -88,7 +73,7 @@ public class TableConfigTunerRegistry {
}
}
}
- });
+ }
_init = true;
LOGGER.info("Initialized TableConfigTunerRegistry with {} tuners: {} in {}
ms", CONFIG_TUNER_MAP.size(),
diff --git
a/pinot-minion/src/main/java/org/apache/pinot/minion/MinionAdminApiApplication.java
b/pinot-minion/src/main/java/org/apache/pinot/minion/MinionAdminApiApplication.java
index 4616f06432..97eae3ff7d 100644
---
a/pinot-minion/src/main/java/org/apache/pinot/minion/MinionAdminApiApplication.java
+++
b/pinot-minion/src/main/java/org/apache/pinot/minion/MinionAdminApiApplication.java
@@ -74,9 +74,7 @@ public class MinionAdminApiApplication extends ResourceConfig
{
} catch (IOException e) {
throw new RuntimeException("Failed to start http server", e);
}
- synchronized (PinotReflectionUtils.getReflectionLock()) {
- setupSwagger();
- }
+ PinotReflectionUtils.runWithLock(this::setupSwagger);
}
private void setupSwagger() {
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderRegistry.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderRegistry.java
index 407ef32419..0e43826fd0 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderRegistry.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderRegistry.java
@@ -21,13 +21,7 @@ package org.apache.pinot.segment.spi.loader;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
-import org.reflections.Reflections;
-import org.reflections.scanners.ResourcesScanner;
-import org.reflections.scanners.SubTypesScanner;
-import org.reflections.scanners.TypeAnnotationsScanner;
-import org.reflections.util.ClasspathHelper;
-import org.reflections.util.ConfigurationBuilder;
-import org.reflections.util.FilterBuilder;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -46,12 +40,10 @@ public class SegmentDirectoryLoaderRegistry {
static {
long startTime = System.currentTimeMillis();
- Reflections reflections = new Reflections(
- new
ConfigurationBuilder().setUrls(ClasspathHelper.forPackage("org.apache.pinot.segment"))
- .filterInputsBy(new FilterBuilder.Include(".*\\.loader\\..*"))
- .setScanners(new ResourcesScanner(), new TypeAnnotationsScanner(),
new SubTypesScanner()));
- Set<Class<?>> classes =
reflections.getTypesAnnotatedWith(SegmentLoader.class);
- classes.forEach(loaderClass -> {
+ Set<Class<?>> loaderClasses =
+
PinotReflectionUtils.getClassesThroughReflection("org.apache.pinot.segment",
".*\\.loader\\..*",
+ SegmentLoader.class);
+ for (Class<?> loaderClass : loaderClasses) {
SegmentLoader segmentLoaderAnnotation =
loaderClass.getAnnotation(SegmentLoader.class);
if (segmentLoaderAnnotation.enabled()) {
if (segmentLoaderAnnotation.name().isEmpty()) {
@@ -69,7 +61,7 @@ public class SegmentDirectoryLoaderRegistry {
}
}
}
- });
+ }
LOGGER.info("Initialized SegmentDirectoryLoaderRegistry with {}
segmentDirectoryLoaders: {} in {} ms",
SEGMENT_DIRECTORY_LOADER_MAP.size(),
SEGMENT_DIRECTORY_LOADER_MAP.keySet(),
diff --git
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/AdminApiApplication.java
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/AdminApiApplication.java
index aa645005a3..dec6ca8085 100644
---
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/AdminApiApplication.java
+++
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/AdminApiApplication.java
@@ -101,15 +101,13 @@ public class AdminApiApplication extends ResourceConfig {
if
(pinotConfiguration.getProperty(CommonConstants.Server.CONFIG_OF_SWAGGER_SERVER_ENABLED,
CommonConstants.Server.DEFAULT_SWAGGER_SERVER_ENABLED)) {
LOGGER.info("Starting swagger for the Pinot server.");
- synchronized (PinotReflectionUtils.getReflectionLock()) {
- setupSwagger(_httpServer, pinotConfiguration);
- }
+ PinotReflectionUtils.runWithLock(() -> setupSwagger(pinotConfiguration));
}
_started = true;
return true;
}
- private void setupSwagger(HttpServer httpServer, PinotConfiguration
pinotConfiguration) {
+ private void setupSwagger(PinotConfiguration pinotConfiguration) {
BeanConfig beanConfig = new BeanConfig();
beanConfig.setTitle("Pinot Server API");
beanConfig.setDescription("APIs for accessing Pinot server information");
@@ -133,13 +131,13 @@ public class AdminApiApplication extends ResourceConfig {
CLStaticHttpHandler staticHttpHandler =
new CLStaticHttpHandler(AdminApiApplication.class.getClassLoader(),
"/api/");
// map both /api and /help to swagger docs. /api because it looks nice.
/help for backward compatibility
- httpServer.getServerConfiguration().addHttpHandler(staticHttpHandler,
"/api/");
- httpServer.getServerConfiguration().addHttpHandler(staticHttpHandler,
"/help/");
+ _httpServer.getServerConfiguration().addHttpHandler(staticHttpHandler,
"/api/");
+ _httpServer.getServerConfiguration().addHttpHandler(staticHttpHandler,
"/help/");
URL swaggerDistLocation =
AdminApiApplication.class.getClassLoader().getResource("META-INF/resources/webjars/swagger-ui/3.23.11/");
CLStaticHttpHandler swaggerDist = new CLStaticHttpHandler(new
URLClassLoader(new URL[]{swaggerDistLocation}));
- httpServer.getServerConfiguration().addHttpHandler(swaggerDist,
"/swaggerui-dist/");
+ _httpServer.getServerConfiguration().addHttpHandler(swaggerDist,
"/swaggerui-dist/");
}
public void stop() {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotReflectionUtils.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotReflectionUtils.java
index 20ee074320..bcc55cd530 100644
---
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotReflectionUtils.java
+++
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotReflectionUtils.java
@@ -19,28 +19,98 @@
package org.apache.pinot.spi.utils;
import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Set;
import org.reflections.Reflections;
-import org.reflections.scanners.TypeAnnotationsScanner;
+import org.reflections.scanners.MethodAnnotationsScanner;
import org.reflections.util.ClasspathHelper;
import org.reflections.util.ConfigurationBuilder;
import org.reflections.util.FilterBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class PinotReflectionUtils {
private PinotReflectionUtils() {
}
- private static final String PINOT_PACKAGE_PREFIX = "org.apache.pinot";
+ private static final Logger LOGGER =
LoggerFactory.getLogger(PinotReflectionUtils.class);
+ private static final String PINOT_PACKAGE_NAME = "org.apache.pinot";
+
+ // We use a lock to prevent multiple threads accessing the same jar in the
same time which can cause exception
+ // See https://github.com/ronmamo/reflections/issues/81 for more details
private static final Object REFLECTION_LOCK = new Object();
- public static Set<Class<?>> getClassesThroughReflection(final String
regexPattern,
- final Class<? extends Annotation> annotation) {
- synchronized (getReflectionLock()) {
- Reflections reflections = new Reflections(
- new
ConfigurationBuilder().setUrls(ClasspathHelper.forPackage(PINOT_PACKAGE_PREFIX))
- .filterInputsBy(new
FilterBuilder.Include(regexPattern)).setScanners(new TypeAnnotationsScanner()));
- return reflections.getTypesAnnotatedWith(annotation, true);
+ public static Set<Class<?>> getClassesThroughReflection(String regexPattern,
Class<? extends Annotation> annotation) {
+ return getClassesThroughReflection(PINOT_PACKAGE_NAME, regexPattern,
annotation);
+ }
+
+ public static Set<Class<?>> getClassesThroughReflection(String packageName,
String regexPattern,
+ Class<? extends Annotation> annotation) {
+ try {
+ synchronized (REFLECTION_LOCK) {
+ return new Reflections(new
ConfigurationBuilder().setUrls(ClasspathHelper.forPackage(packageName))
+ .filterInputsBy(new
FilterBuilder.Include(regexPattern))).getTypesAnnotatedWith(annotation);
+ }
+ } catch (Throwable t) {
+ // Log an error then re-throw it because this method is usually called
in a static block, where exception might
+ // not be properly handled
+ LOGGER.error("Error scanning classes within package: '{}' with regex
pattern: '{}', annotation: {}", packageName,
+ regexPattern, annotation.getSimpleName(), t);
+ throw t;
+ }
+ }
+
+ public static Set<Class<?>> getClassesThroughReflection(List<String>
packages, String regexPattern,
+ Class<? extends Annotation> annotation) {
+ try {
+ synchronized (REFLECTION_LOCK) {
+ List<URL> urls = new ArrayList<>();
+ for (String packageName : packages) {
+ urls.addAll(ClasspathHelper.forPackage(packageName));
+ }
+ return new Reflections(new ConfigurationBuilder().setUrls(urls)
+ .filterInputsBy(new
FilterBuilder.Include(regexPattern))).getTypesAnnotatedWith(annotation);
+ }
+ } catch (Throwable t) {
+ // Log an error then re-throw it because this method is usually called
in a static block, where exception might
+ // not be properly handled
+ LOGGER.error("Error scanning classes within packages: {} with regex
pattern: '{}', annotation: {}", packages,
+ regexPattern, annotation.getSimpleName(), t);
+ throw t;
+ }
+ }
+
+ public static Set<Method> getMethodsThroughReflection(String regexPattern,
Class<? extends Annotation> annotation) {
+ return getMethodsThroughReflection(PINOT_PACKAGE_NAME, regexPattern,
annotation);
+ }
+
+ public static Set<Method> getMethodsThroughReflection(String packageName,
String regexPattern,
+ Class<? extends Annotation> annotation) {
+ try {
+ synchronized (REFLECTION_LOCK) {
+ return new Reflections(new
ConfigurationBuilder().setUrls(ClasspathHelper.forPackage(packageName))
+ .filterInputsBy(new FilterBuilder.Include(regexPattern))
+ .setScanners(new
MethodAnnotationsScanner())).getMethodsAnnotatedWith(annotation);
+ }
+ } catch (Throwable t) {
+ // Log an error then re-throw it because this method is usually called
in a static block, where exception might
+ // not be properly handled
+ LOGGER.error("Error scanning methods within package: '{}' with regex
pattern: '{}', annotation: {}", packageName,
+ regexPattern, annotation.getSimpleName(), t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes the given runnable within the reflection lock.
+ */
+ public static void runWithLock(Runnable runnable) {
+ synchronized (REFLECTION_LOCK) {
+ runnable.run();
}
}
@@ -48,8 +118,9 @@ public class PinotReflectionUtils {
* Due to the multi-threading issue in org.reflections.vfs.ZipDir, we need
to put a lock before calling the
* reflection related methods.
*
- * @return
+ * Deprecated: use {@link #runWithLock(Runnable)} instead
*/
+ @Deprecated
public static Object getReflectionLock() {
return REFLECTION_LOCK;
}
diff --git
a/pinot-tools/src/main/java/org/apache/pinot/tools/service/PinotServiceManagerAdminApiApplication.java
b/pinot-tools/src/main/java/org/apache/pinot/tools/service/PinotServiceManagerAdminApiApplication.java
index 20f5392401..c04286d1bf 100644
---
a/pinot-tools/src/main/java/org/apache/pinot/tools/service/PinotServiceManagerAdminApiApplication.java
+++
b/pinot-tools/src/main/java/org/apache/pinot/tools/service/PinotServiceManagerAdminApiApplication.java
@@ -58,9 +58,7 @@ public class PinotServiceManagerAdminApiApplication extends
ResourceConfig {
Preconditions.checkArgument(httpPort > 0);
_baseUri = URI.create("http://0.0.0.0:"; + httpPort + "/");
_httpServer = GrizzlyHttpServerFactory.createHttpServer(_baseUri, this);
- synchronized (PinotReflectionUtils.getReflectionLock()) {
- setupSwagger();
- }
+ PinotReflectionUtils.runWithLock(this::setupSwagger);
}
private void setupSwagger() {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
