Address feedback

prestodb · Oct 13, 2024 · 7d39e04 · 7d39e04
1 parent d4edfdd
commit 7d39e04
Show file tree

Hide file tree

Showing 7 changed files with 27 additions and 34 deletions.
diff --git a/presto-main/src/main/java/com/facebook/presto/connector/ConnectorAwareNodeManager.java b/presto-main/src/main/java/com/facebook/presto/connector/ConnectorAwareNodeManager.java
@@ -43,6 +43,7 @@ public Set<Node> getAllNodes()
         return ImmutableSet.<Node>builder()
                 .addAll(getWorkerNodes())
                 .addAll(nodeManager.getCoordinators())
+                .addAll(nodeManager.getCoordinatorSidecars())
                 .build();
     }
 

diff --git a/presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java b/presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java
@@ -40,14 +40,17 @@
 import com.facebook.presto.execution.warnings.WarningCollectorModule;
 import com.facebook.presto.metadata.Catalog;
 import com.facebook.presto.metadata.CatalogManager;
+import com.facebook.presto.metadata.DiscoveryNodeManager;
 import com.facebook.presto.metadata.InMemoryNodeManager;
+import com.facebook.presto.metadata.InternalNodeManager;
 import com.facebook.presto.metadata.StaticCatalogStore;
 import com.facebook.presto.metadata.StaticFunctionNamespaceStore;
 import com.facebook.presto.security.AccessControlManager;
 import com.facebook.presto.security.AccessControlModule;
 import com.facebook.presto.server.security.PasswordAuthenticatorManager;
 import com.facebook.presto.server.security.ServerSecurityModule;
 import com.facebook.presto.spi.ConnectorId;
+import com.facebook.presto.spi.NodeManager;
 import com.facebook.presto.sql.analyzer.FeaturesConfig;
 import com.facebook.presto.sql.parser.SqlParserOptions;
 import com.facebook.presto.sql.planner.sanity.PlanCheckerProviderManager;
@@ -181,8 +184,9 @@ public void run()
             injector.getInstance(TracerProviderManager.class).loadTracerProvider();
             injector.getInstance(NodeStatusNotificationManager.class).loadNodeStatusNotificationProvider();
             injector.getInstance(GracefulShutdownHandler.class).loadNodeStatusNotification();
-            ConnectorAwareNodeManager connectorAwareNodeManager = new ConnectorAwareNodeManager(injector.getInstance(InMemoryNodeManager.class), "", new ConnectorId("<NA>"));
             PlanCheckerProviderManager planCheckerProviderManager = injector.getInstance(PlanCheckerProviderManager.class);
+            InternalNodeManager nodeManager = injector.getInstance(DiscoveryNodeManager.class);
+            ConnectorAwareNodeManager connectorAwareNodeManager = new ConnectorAwareNodeManager(nodeManager, "", new ConnectorId("<NA>"));
             planCheckerProviderManager.loadPlanCheckerProviders(connectorAwareNodeManager);
 
             startAssociatedProcesses(injector);

diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/PlanChecker.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/PlanChecker.java
@@ -107,7 +107,15 @@ public void validatePlanFragment(PlanFragment planFragment, Session session, Met
         checkers.get(Stage.FRAGMENT).forEach(checker -> checker.validateFragment(planFragment, session, metadata, warningCollector));
         for (PlanCheckerProvider provider : planCheckerProviderManager.getPlanCheckerProviders()) {
             for (com.facebook.presto.spi.plan.PlanChecker checker : provider.getFragmentPlanCheckers()) {
-                checker.validateFragment(toSimplePlanFragment(planFragment), warningCollector);
+                checker.validateFragment(new SimplePlanFragment(
+                        planFragment.getId(),
+                        planFragment.getRoot(),
+                        planFragment.getVariables(),
+                        planFragment.getPartitioning(),
+                        planFragment.getTableScanSchedulingOrder(),
+                        planFragment.getPartitioningScheme(),
+                        planFragment.getStageExecutionDescriptor(),
+                        planFragment.isOutputTableWriterFragment()), warningCollector);
             }
         }
     }
@@ -126,17 +134,4 @@ private enum Stage
     {
         INTERMEDIATE, FINAL, FRAGMENT
     }
-
-    private static SimplePlanFragment toSimplePlanFragment(PlanFragment planFragment)
-    {
-        return new SimplePlanFragment(
-                planFragment.getId(),
-                planFragment.getRoot(),
-                planFragment.getVariables(),
-                planFragment.getPartitioning(),
-                planFragment.getTableScanSchedulingOrder(),
-                planFragment.getPartitioningScheme(),
-                planFragment.getStageExecutionDescriptor(),
-                planFragment.isOutputTableWriterFragment());
-    }
 }
diff --git a/...c/main/java/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanChecker.java b/...c/main/java/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanChecker.java
@@ -21,7 +21,6 @@
 import com.facebook.presto.spi.PrestoException;
 import com.facebook.presto.spi.TableHandle;
 import com.facebook.presto.spi.WarningCollector;
-import com.facebook.presto.spi.plan.FilterNode;
 import com.facebook.presto.spi.plan.PlanChecker;
 import com.facebook.presto.spi.plan.PlanNode;
 import com.facebook.presto.spi.plan.PlanVisitor;
@@ -49,10 +48,6 @@ public final class NativePlanChecker
         implements PlanChecker
 {
     private static final Logger LOG = Logger.get(NativePlanChecker.class);
-    private static final String PRESTO_QUERY_ID = "X-Presto-Query-Id";
-    private static final String PRESTO_TIME_ZONE = "X-Presto-Time-Zone";
-    private static final String PRESTO_SYSTEM_PROPERTY = "X-Presto-System-Property";
-    private static final String PRESTO_CATALOG_PROPERTY = "X-Presto-Catalog-Property";
     private static final MediaType JSON_CONTENT_TYPE = MediaType.parse("application/json; charset=utf-8");
     public static final String PLAN_CONVERSION_ENDPOINT = "/v1/velox/plan";
 
@@ -96,7 +91,7 @@ public void validateFragment(SimplePlanFragment planFragment, WarningCollector w
             if (!planFragment.getPartitioning().isCoordinatorOnly() && !isInternalSystemConnector(planFragment.getRoot())) {
                 runValidation(planFragment);
             }
-            else if (LOG.isDebugEnabled()) {
+            else {
                 LOG.debug("Skipping Native Plan Validation for plan fragment id: %s", planFragment.getId());
             }
         }
@@ -137,7 +132,6 @@ private void runValidation(SimplePlanFragment planFragment)
         try (Response response = httpClient.newCall(request).execute()) {
             if (!response.isSuccessful()) {
                 String responseBody = response.body() != null ? response.body().string() : "{}";
-                LOG.error("Native plan checker failed with code: %d, response: %s", response.code(), responseBody);
                 if (config.isQueryFailOnError()) {
                     throw new PrestoException(QUERY_REJECTED, "Query failed by native plan checker with code: " + response.code() + ", response: " + responseBody);
                 }
@@ -167,15 +161,14 @@ public Boolean visitTableScan(TableScanNode tableScan, Void context)
             return ConnectorId.isInternalSystemConnector(handle.getConnectorId());
         }
 
-        @Override
-        public Boolean visitFilter(FilterNode filter, Void context)
-        {
-            return filter.getSource().accept(this, context);
-        }
-
         @Override
         public Boolean visitPlan(PlanNode node, Void context)
         {
+            for (PlanNode child : node.getSources()) {
+                if (child.accept(this, context)) {
+                    return true;
+                }
+            }
             return false;
         }
     }

diff --git a/.../java/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerConfig.java b/.../java/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerConfig.java
@@ -27,7 +27,7 @@ public boolean isPlanValidationEnabled()
         return enabled;
     }
 
-    @Config("plan-validation-enabled")
+    @Config("native-plan-checker.plan-validation-enabled")
     @ConfigDescription("Set true to enable native plan validation")
     public NativePlanCheckerConfig setPlanValidationEnabled(boolean enabled)
     {
@@ -40,7 +40,7 @@ public boolean isQueryFailOnError()
         return queryFailOnError;
     }
 
-    @Config("query-fail-on-error")
+    @Config("native-plan-checker.query-fail-on-error")
     @ConfigDescription("Set true to fail the query if plan does not pass native validation, false will log error only")
     public NativePlanCheckerConfig setQueryFailOnError(boolean queryFailOnError)
     {

diff --git a/...ava/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerProvider.java b/...ava/com/facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerProvider.java
@@ -19,9 +19,9 @@
 import com.facebook.presto.spi.plan.PlanChecker;
 import com.facebook.presto.spi.plan.PlanCheckerProvider;
 import com.facebook.presto.spi.plan.SimplePlanFragment;
+import com.google.common.collect.ImmutableList;
 import com.google.inject.Inject;
 
-import java.util.Collections;
 import java.util.List;
 
 import static java.util.Objects.requireNonNull;
@@ -45,7 +45,7 @@ public NativePlanCheckerProvider(NodeManager nodeManager, JsonCodec<SimplePlanFr
     public List<PlanChecker> getFragmentPlanCheckers()
     {
         return config.isPlanValidationEnabled() ?
-                Collections.singletonList(new NativePlanChecker(nodeManager, planFragmentJsonCodec, config)) :
-                Collections.emptyList();
+                ImmutableList.of(new NativePlanChecker(nodeManager, planFragmentJsonCodec, config)) :
+                ImmutableList.of();
     }
 }
diff --git a/.../facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerProviderFactory.java b/.../facebook/presto/plancheckerproviders/nativechecker/NativePlanCheckerProviderFactory.java
@@ -41,7 +41,7 @@
 public class NativePlanCheckerProviderFactory
         implements PlanCheckerProviderFactory
 {
-    private static final File NATIVE_PLAN_CHECKER_PROVIDER_CONFIG = new File("etc/native-plan-checker-provider.properties");
+    private static final File NATIVE_PLAN_CHECKER_PROVIDER_CONFIG = new File("etc/plan-checker-providers/native-plan-checker.properties");
     private final ClassLoader classLoader;
 
     public NativePlanCheckerProviderFactory(ClassLoader classLoader)