From d960aef2569d5b7b00144fdba319904b49e14785 Mon Sep 17 00:00:00 2001
From: Wenlei Xie <wenlei.xie@gmail.com>
Date: Sun, 30 Jun 2019 13:44:55 -0700
Subject: [PATCH] Improve error message when resource group selector cannot be
 loaded

---
 .../DbResourceGroupConfigurationManager.java  | 35 ++++++++++++-------
 ...stDbResourceGroupConfigurationManager.java |  4 +--
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/presto-resource-group-managers/src/main/java/com/facebook/presto/resourceGroups/db/DbResourceGroupConfigurationManager.java b/presto-resource-group-managers/src/main/java/com/facebook/presto/resourceGroups/db/DbResourceGroupConfigurationManager.java
index 091dec94eacb..aef9ee7a46ee 100644
--- a/presto-resource-group-managers/src/main/java/com/facebook/presto/resourceGroups/db/DbResourceGroupConfigurationManager.java
+++ b/presto-resource-group-managers/src/main/java/com/facebook/presto/resourceGroups/db/DbResourceGroupConfigurationManager.java
@@ -63,8 +63,10 @@
 import static com.google.common.base.Preconditions.checkState;
 import static io.airlift.concurrent.Threads.daemonThreadsNamed;
 import static io.airlift.units.Duration.succinctNanos;
+import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;
 import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
 public class DbResourceGroupConfigurationManager
         extends AbstractResourceConfigurationManager
@@ -116,9 +118,8 @@ protected Optional<Duration> getCpuQuotaPeriod()
     @Override
     protected List<ResourceGroupSpec> getRootGroups()
     {
-        if (lastRefresh.get() == 0) {
-            throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Root groups cannot be fetched from database");
-        }
+        checkMaxRefreshInterval();
+
         if (this.selectors.get().isEmpty()) {
             throw new PrestoException(CONFIGURATION_INVALID, "No root groups are configured");
         }
@@ -156,9 +157,8 @@ public void configure(ResourceGroup group, SelectionContext<VariableMap> criteri
     @Override
     public Optional<SelectionContext<VariableMap>> match(SelectionCriteria criteria)
     {
-        if (lastRefresh.get() == 0) {
-            throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Selectors cannot be fetched from database");
-        }
+        checkMaxRefreshInterval();
+
         if (selectors.get().isEmpty()) {
             throw new PrestoException(CONFIGURATION_INVALID, "No selectors are configured");
         }
@@ -173,9 +173,8 @@ public Optional<SelectionContext<VariableMap>> match(SelectionCriteria criteria)
     @VisibleForTesting
     public List<ResourceGroupSelector> getSelectors()
     {
-        if (lastRefresh.get() == 0) {
-            throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Selectors cannot be fetched from database");
-        }
+        checkMaxRefreshInterval();
+
         if (selectors.get().isEmpty()) {
             throw new PrestoException(CONFIGURATION_INVALID, "No selectors are configured");
         }
@@ -237,11 +236,11 @@ public synchronized void load()
             lastRefresh.set(System.nanoTime());
         }
         catch (Throwable e) {
-            if (succinctNanos(System.nanoTime() - lastRefresh.get()).compareTo(maxRefreshInterval) > 0) {
-                lastRefresh.set(0);
-            }
             refreshFailures.update(1);
             log.error(e, "Error loading configuration from db");
+            if (lastRefresh.get() != 0) {
+                log.debug("Last successful configuration loading was %s ago", succinctNanos(System.nanoTime() - lastRefresh.get()).toString());
+            }
         }
     }
 
@@ -367,6 +366,18 @@ private ResourceGroup getRootGroup(ResourceGroupId groupId)
         return groups.get(groupId);
     }
 
+    private void checkMaxRefreshInterval()
+    {
+        if (System.nanoTime() - lastRefresh.get() > maxRefreshInterval.toMillis() * MILLISECONDS.toNanos(1)) {
+            String message = "Resource group configuration cannot be fetched from database.";
+            if (lastRefresh.get() != 0) {
+                message += format(" Current resource group configuration is loaded %s ago", succinctNanos(System.nanoTime() - lastRefresh.get()).toString());
+            }
+
+            throw new PrestoException(CONFIGURATION_UNAVAILABLE, message);
+        }
+    }
+
     @Managed
     @Nested
     public CounterStat getRefreshFailures()
diff --git a/presto-resource-group-managers/src/test/java/com/facebook/presto/resourceGroups/db/TestDbResourceGroupConfigurationManager.java b/presto-resource-group-managers/src/test/java/com/facebook/presto/resourceGroups/db/TestDbResourceGroupConfigurationManager.java
index 22f6024b79a7..89d3e4c38ee1 100644
--- a/presto-resource-group-managers/src/test/java/com/facebook/presto/resourceGroups/db/TestDbResourceGroupConfigurationManager.java
+++ b/presto-resource-group-managers/src/test/java/com/facebook/presto/resourceGroups/db/TestDbResourceGroupConfigurationManager.java
@@ -329,7 +329,7 @@ public void testRefreshInterval()
             fail("Expected unavailable configuration exception");
         }
         catch (Exception e) {
-            assertEquals(e.getMessage(), "Selectors cannot be fetched from database");
+            assertTrue(e.getMessage().startsWith("Resource group configuration cannot be fetched from database."));
         }
 
         try {
@@ -337,7 +337,7 @@ public void testRefreshInterval()
             fail("Expected unavailable configuration exception");
         }
         catch (Exception e) {
-            assertEquals(e.getMessage(), "Root groups cannot be fetched from database");
+            assertTrue(e.getMessage().startsWith("Resource group configuration cannot be fetched from database."));
         }
 
         manager.destroy();