[native] Validate sidecar function signatures against plugin loaded function signatures at startup

Author: pdabre12

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInPluginFunctionNamespaceManager.java

@@ -31,6 +31,11 @@ public BuiltInPluginFunctionNamespaceManager(FunctionAndTypeManager functionAndT
         super(functionAndTypeManager);
     }
 
+    public void triggerConflictCheckWithBuiltInFunctions()
+    {
+        checkForNamingConflicts(this.getFunctionsFromDefaultNamespace());
+    }
+
     @Override
     public synchronized void registerBuiltInSpecialFunctions(List<? extends SqlFunction> functions)
     {

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInSpecialFunctionNamespaceManager.java

@@ -58,7 +58,7 @@ public abstract class BuiltInSpecialFunctionNamespaceManager
 {
     protected volatile FunctionMap functions = new FunctionMap();
     private final FunctionAndTypeManager functionAndTypeManager;
-    private final Supplier<FunctionMap> cachedFunctions =
+    protected final Supplier<FunctionMap> cachedFunctions =
             Suppliers.memoize(this::createFunctionMap);
     private final LoadingCache<Signature, SpecializedFunctionKey> specializedFunctionKeyCache;
     private final LoadingCache<SpecializedFunctionKey, ScalarFunctionImplementation> specializedScalarCache;
@@ -212,12 +212,16 @@ public ScalarFunctionImplementation getScalarFunctionImplementation(FunctionHand
         return getScalarFunctionImplementation(((BuiltInFunctionHandle) functionHandle).getSignature());
     }
 
-    private synchronized FunctionMap createFunctionMap()
+    protected Collection<? extends SqlFunction> getFunctionsFromDefaultNamespace()
     {
         Optional<FunctionNamespaceManager<?>> functionNamespaceManager =
                 functionAndTypeManager.getServingFunctionNamespaceManager(functionAndTypeManager.getDefaultNamespace());
         checkArgument(functionNamespaceManager.isPresent(), "Cannot find function namespace for catalog '%s'", functionAndTypeManager.getDefaultNamespace().getCatalogName());
-        checkForNamingConflicts(functionNamespaceManager.get().listFunctions(Optional.empty(), Optional.empty()));
+        return functionNamespaceManager.get().listFunctions(Optional.empty(), Optional.empty());
+    }
+
+    private synchronized FunctionMap createFunctionMap()
+    {
         return functions;
     }
 

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java

@@ -979,7 +979,12 @@ public SpecializedFunctionKey getSpecializedFunctionKey(Signature signature, Col
         return builtInTypeAndFunctionNamespaceManager.doGetSpecializedFunctionKeyForMagicLiteralFunctions(signature, this);
     }
 
-    public CatalogSchemaName configureDefaultNamespace(String defaultNamespacePrefixString)
+    public BuiltInPluginFunctionNamespaceManager getBuiltInPluginFunctionNamespaceManager()
+    {
+        return builtInPluginFunctionNamespaceManager;
+    }
+
+    private CatalogSchemaName configureDefaultNamespace(String defaultNamespacePrefixString)
     {
         if (!defaultNamespacePrefixString.matches(DEFAULT_NAMESPACE_PREFIX_PATTERN.pattern())) {
             throw new PrestoException(GENERIC_USER_ERROR, format("Default namespace prefix string should be in the form of 'catalog.schema', found: %s", defaultNamespacePrefixString));

presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java

@@ -209,6 +209,9 @@ public void run()
             injector.getInstance(ClientRequestFilterManager.class).loadClientRequestFilters();
             injector.getInstance(ExpressionOptimizerManager.class).loadExpressionOptimizerFactories();
 
+            injector.getInstance(FunctionAndTypeManager.class)
+                    .getBuiltInPluginFunctionNamespaceManager().triggerConflictCheckWithBuiltInFunctions();
+
             startAssociatedProcesses(injector);
 
             injector.getInstance(Announcer.class).start();

presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java

@@ -548,6 +548,12 @@ public void installCoordinatorPlugin(CoordinatorPlugin plugin)
         pluginManager.installCoordinatorPlugin(plugin);
     }
 
+    public void triggerConflictCheckWithBuiltInFunctions()
+    {
+        metadata.getFunctionAndTypeManager()
+                .getBuiltInPluginFunctionNamespaceManager().triggerConflictCheckWithBuiltInFunctions();
+    }
+
     public DispatchManager getDispatchManager()
     {
         return dispatchManager;

presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionDefinitionProvider.java

@@ -14,26 +14,24 @@
 package com.facebook.presto.sidecar.functionNamespace;
 
 import com.facebook.airlift.http.client.HttpClient;
-import com.facebook.airlift.http.client.HttpUriBuilder;
 import com.facebook.airlift.http.client.Request;
 import com.facebook.airlift.json.JsonCodec;
 import com.facebook.airlift.log.Logger;
 import com.facebook.presto.functionNamespace.JsonBasedUdfFunctionMetadata;
 import com.facebook.presto.functionNamespace.UdfFunctionSignatureMap;
 import com.facebook.presto.sidecar.ForSidecarInfo;
-import com.facebook.presto.spi.Node;
 import com.facebook.presto.spi.NodeManager;
 import com.facebook.presto.spi.PrestoException;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import com.google.inject.Inject;
 
-import java.net.URI;
 import java.util.List;
 import java.util.Map;
 
 import static com.facebook.airlift.http.client.JsonResponseHandler.createJsonResponseHandler;
 import static com.facebook.airlift.http.client.Request.Builder.prepareGet;
+import static com.facebook.presto.builtin.tools.NativeSidecarFunctionRegistryTool.getSidecarLocationOnStartup;
 import static com.facebook.presto.spi.StandardErrorCode.INVALID_ARGUMENTS;
 import static java.util.Objects.requireNonNull;
 
@@ -42,27 +40,29 @@ public class NativeFunctionDefinitionProvider
 {
     private static final Logger log = Logger.get(NativeFunctionDefinitionProvider.class);
     private final JsonCodec<Map<String, List<JsonBasedUdfFunctionMetadata>>> nativeFunctionSignatureMapJsonCodec;
-    private final NodeManager nodeManager;
     private final HttpClient httpClient;
-    private static final String FUNCTION_SIGNATURES_ENDPOINT = "/v1/functions";
+    private final NativeFunctionNamespaceManagerConfig config;
 
     @Inject
     public NativeFunctionDefinitionProvider(
             @ForSidecarInfo HttpClient httpClient,
             JsonCodec<Map<String, List<JsonBasedUdfFunctionMetadata>>> nativeFunctionSignatureMapJsonCodec,
-            NodeManager nodeManager)
+            NativeFunctionNamespaceManagerConfig config)
     {
         this.nativeFunctionSignatureMapJsonCodec =
                 requireNonNull(nativeFunctionSignatureMapJsonCodec, "nativeFunctionSignatureMapJsonCodec is null");
-        this.nodeManager = requireNonNull(nodeManager, "nodeManager is null");
-        this.httpClient = requireNonNull(httpClient, "typeManager is null");
+        this.httpClient = requireNonNull(httpClient, "httpClient is null");
+        this.config = requireNonNull(config, "config is null");
     }
 
     @Override
     public UdfFunctionSignatureMap getUdfDefinition(NodeManager nodeManager)
     {
         try {
-            Request request = prepareGet().setUri(getSidecarLocation()).build();
+            Request request =
+                    prepareGet().setUri(
+                            getSidecarLocationOnStartup(
+                                    nodeManager, config.getSidecarNumRetries(), config.getSidecarRetryDelay().toMillis())).build();
             Map<String, List<JsonBasedUdfFunctionMetadata>> nativeFunctionSignatureMap = httpClient.execute(request, createJsonResponseHandler(nativeFunctionSignatureMapJsonCodec));
             return new UdfFunctionSignatureMap(ImmutableMap.copyOf(nativeFunctionSignatureMap));
         }
@@ -71,15 +71,6 @@ public UdfFunctionSignatureMap getUdfDefinition(NodeManager nodeManager)
         }
     }
 
-    private URI getSidecarLocation()
-    {
-        Node sidecarNode = nodeManager.getSidecarNode();
-        return HttpUriBuilder
-                .uriBuilderFrom(sidecarNode.getHttpUri())
-                .appendPath(FUNCTION_SIGNATURES_ENDPOINT)
-                .build();
-    }
-
     @VisibleForTesting
     public HttpClient getHttpClient()
     {

presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManagerConfig.java

@@ -0,0 +1,52 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.facebook.presto.sidecar.functionNamespace;
+
+import com.facebook.airlift.configuration.Config;
+import com.facebook.airlift.configuration.ConfigDescription;
+import com.facebook.airlift.units.Duration;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+public class NativeFunctionNamespaceManagerConfig
+{
+    private int sidecarNumRetries = 8;
+    private Duration sidecarRetryDelay = new Duration(1, MINUTES);
+
+    public int getSidecarNumRetries()
+    {
+        return sidecarNumRetries;
+    }
+
+    @Config("sidecar.num-retries")
+    @ConfigDescription("Max times to retry fetching sidecar node")
+    public NativeFunctionNamespaceManagerConfig setSidecarNumRetries(int sidecarNumRetries)
+    {
+        this.sidecarNumRetries = sidecarNumRetries;
+        return this;
+    }
+
+    public Duration getSidecarRetryDelay()
+    {
+        return sidecarRetryDelay;
+    }
+
+    @Config("sidecar.retry-delay")
+    @ConfigDescription("Cooldown period to retry when fetching sidecar node fails")
+    public NativeFunctionNamespaceManagerConfig setSidecarRetryDelay(Duration sidecarRetryDelay)
+    {
+        this.sidecarRetryDelay = sidecarRetryDelay;
+        return this;
+    }
+}

presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManagerModule.java

@@ -37,7 +37,6 @@ public class NativeFunctionNamespaceManagerModule
         implements Module
 {
     private final String catalogName;
-
     private final NodeManager nodeManager;
     private final FunctionMetadataManager functionMetadataManager;
 
@@ -55,6 +54,7 @@ public void configure(Binder binder)
 
         configBinder(binder).bindConfig(SqlInvokedFunctionNamespaceManagerConfig.class);
         configBinder(binder).bindConfig(SqlFunctionLanguageConfig.class);
+        configBinder(binder).bindConfig(NativeFunctionNamespaceManagerConfig.class);
         binder.bind(FunctionDefinitionProvider.class).to(NativeFunctionDefinitionProvider.class).in(SINGLETON);
         binder.bind(new TypeLiteral<JsonCodec<Map<String, List<JsonBasedUdfFunctionMetadata>>>>() {})
                 .toInstance(new JsonCodecFactory().mapJsonCodec(String.class, listJsonCodec(JsonBasedUdfFunctionMetadata.class)));

presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/functionNamespace/TestNativeFunctionNamespaceManagerConfig.java

@@ -0,0 +1,51 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.facebook.presto.sidecar.functionNamespace;
+
+import com.facebook.airlift.units.Duration;
+import com.google.common.collect.ImmutableMap;
+import org.testng.annotations.Test;
+
+import java.util.Map;
+
+import static com.facebook.airlift.configuration.testing.ConfigAssertions.assertFullMapping;
+import static com.facebook.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults;
+import static com.facebook.airlift.configuration.testing.ConfigAssertions.recordDefaults;
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+public class TestNativeFunctionNamespaceManagerConfig
+{
+    @Test
+    public void testDefaults()
+    {
+        assertRecordedDefaults(recordDefaults(NativeFunctionNamespaceManagerConfig.class)
+                .setSidecarNumRetries(8)
+                .setSidecarRetryDelay(new Duration(1, MINUTES)));
+    }
+
+    @Test
+    public void testExplicitPropertyMappings()
+    {
+        Map<String, String> properties = new ImmutableMap.Builder<String, String>()
+                .put("sidecar.num-retries", "15")
+                .put("sidecar.retry-delay", "5m")
+                .build();
+
+        NativeFunctionNamespaceManagerConfig expected = new NativeFunctionNamespaceManagerConfig()
+                .setSidecarNumRetries(15)
+                .setSidecarRetryDelay(new Duration(5, MINUTES));
+
+        assertFullMapping(properties, expected);
+    }
+}

presto-tests/src/test/java/com/facebook/presto/functions/TestPluginLoadedDuplicateSqlInvokedFunctions.java

@@ -22,12 +22,9 @@
 import org.testng.annotations.Test;
 
 import java.util.Set;
-import java.util.regex.Pattern;
 
-import static com.facebook.presto.metadata.BuiltInTypeAndFunctionNamespaceManager.JAVA_BUILTIN_NAMESPACE;
 import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
-import static java.lang.String.format;
-import static org.testng.Assert.fail;
+import static org.testng.Assert.assertThrows;
 
 public class TestPluginLoadedDuplicateSqlInvokedFunctions
 {
@@ -45,20 +42,6 @@ public void setup()
                 .build());
     }
 
-    public void assertInvalidFunction(String expr, String exceptionPattern)
-    {
-        try {
-            client.execute("SELECT " + expr);
-            fail("Function expected to fail but not");
-        }
-        catch (Exception e) {
-            if (!(e.getMessage().matches(exceptionPattern))) {
-                fail(format("Expected exception message '%s' to match '%s' but not",
-                        e.getMessage(), exceptionPattern));
-            }
-        }
-    }
-
     private static class TestDuplicateFunctionsPlugin
             implements Plugin
     {
@@ -71,10 +54,11 @@ public Set<Class<?>> getSqlInvokedFunctions()
         }
     }
 
+    // As soon as we trigger the conflict check with the built-in functions, an error will be thrown if duplicate signatures are found.
+    // In `PrestoServer.java` this conflict check is triggered implicitly.
     @Test
     public void testDuplicateFunctionsLoaded()
     {
-        assertInvalidFunction(JAVA_BUILTIN_NAMESPACE + ".modulo(10,3)",
-                Pattern.quote(format("java.lang.IllegalArgumentException: Function already registered: %s.array_intersect<T>(array(array(T))):array(T)", JAVA_BUILTIN_NAMESPACE)));
+        assertThrows(IllegalArgumentException.class, () -> server.triggerConflictCheckWithBuiltInFunctions());
     }
 }