[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5531: Fix the failure caused by Reflections in FunctionRegistry

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #5531:
URL: https://github.com/apache/incubator-pinot/pull/5531#discussion_r437743484



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
##########
@@ -39,62 +38,63 @@
  */
 public class FunctionRegistry {
   private static final Logger LOGGER = LoggerFactory.getLogger(FunctionRegistry.class);
-  private static final Map<String, FunctionInfo> _functionInfoMap = new HashMap<>();
+  private static final Map<String, FunctionInfo> FUNCTION_INFO_MAP = new HashMap<>();
+
+  private static boolean _initialized = false;
 
   /**
-   * Given a function name, asserts that a corresponding function was registered during construction and returns it
+   * Initializes the FunctionRegistry by registering the scalar functions via reflection.
+   * NOTE: In order to plugin methods using reflection, the methods should be inside a class that includes ".function."
+   *       in its class path. This convention can significantly reduce the time of class scanning.
    */
-  public static FunctionInfo getFunctionByName(String functionName) {
-    Preconditions.checkArgument(_functionInfoMap.containsKey(functionName.toLowerCase()));
-    return _functionInfoMap.get(functionName.toLowerCase());
+  public static synchronized void init() {
+    if (_initialized) {
+      LOGGER.info("FunctionRegistry is already initialized");
+      return;
+    }
+
+    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) {
+      ScalarFunction scalarFunction = method.getAnnotation(ScalarFunction.class);
+      if (scalarFunction.enabled()) {
+        if (!scalarFunction.name().isEmpty()) {
+          FunctionRegistry.registerFunction(scalarFunction.name(), method);
+        } else {
+          FunctionRegistry.registerFunction(method);
+        }
+      }
+    }
+    LOGGER.info("Initialized FunctionRegistry within {}ms", System.currentTimeMillis() - startTimeMs);

Review comment:
       It was taking several seconds before adding filter on the class path. That is another reason why we should explicitly initialize it instead of waiting for class loader to run the static block. Before the change, the reflection happens when the first query arrives, which means the first query have to wait for several seconds. Right now it should take ~200ms, and it happens in the instance setup.
   Added the map size and functions registered.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org