From 1addea4dbe4603fb68632743b1f95fb2bd5c3f28 Mon Sep 17 00:00:00 2001
From: fjy <fangjin@metamarkets.com>
Date: Mon, 19 May 2014 17:52:34 -0700
Subject: [PATCH] more code review comments

---
 .../io/druid/client/ImmutableDruidServer.java | 25 -------------------
 .../java/io/druid/db/DatabaseRuleManager.java | 17 ++++---------
 .../server/coordinator/DruidCoordinator.java  |  4 +++
 .../helper/DruidCoordinatorRuleRunner.java    | 10 +++-----
 4 files changed, 13 insertions(+), 43 deletions(-)

diff --git a/server/src/main/java/io/druid/client/ImmutableDruidServer.java b/server/src/main/java/io/druid/client/ImmutableDruidServer.java
index 6846d0b89a7..447c9f3edef 100644
--- a/server/src/main/java/io/druid/client/ImmutableDruidServer.java
+++ b/server/src/main/java/io/druid/client/ImmutableDruidServer.java
@@ -101,29 +101,4 @@ public class ImmutableDruidServer
   {
     return segments;
   }
-
-  @Override
-  public boolean equals(Object o)
-  {
-    if (this == o) {
-      return true;
-    }
-    if (o == null || getClass() != o.getClass()) {
-      return false;
-    }
-
-    ImmutableDruidServer that = (ImmutableDruidServer) o;
-
-    if (metadata != null ? !metadata.equals(that.metadata) : that.metadata != null) {
-      return false;
-    }
-
-    return true;
-  }
-
-  @Override
-  public int hashCode()
-  {
-    return metadata != null ? metadata.hashCode() : 0;
-  }
 }
diff --git a/server/src/main/java/io/druid/db/DatabaseRuleManager.java b/server/src/main/java/io/druid/db/DatabaseRuleManager.java
index 682e6fc8d8c..b8cb792b57f 100644
--- a/server/src/main/java/io/druid/db/DatabaseRuleManager.java
+++ b/server/src/main/java/io/druid/db/DatabaseRuleManager.java
@@ -51,7 +51,6 @@ import java.sql.SQLException;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -125,7 +124,7 @@ public class DatabaseRuleManager
   private final Supplier<DatabaseRuleManagerConfig> config;
   private final Supplier<DbTablesConfig> dbTables;
   private final IDBI dbi;
-  private final AtomicReference<ConcurrentHashMap<String, List<Rule>>> rules;
+  private final AtomicReference<ImmutableMap<String, List<Rule>>> rules;
 
   private volatile ScheduledExecutorService exec;
 
@@ -146,8 +145,8 @@ public class DatabaseRuleManager
     this.dbTables = dbTables;
     this.dbi = dbi;
 
-    this.rules = new AtomicReference<ConcurrentHashMap<String, List<Rule>>>(
-        new ConcurrentHashMap<String, List<Rule>>()
+    this.rules = new AtomicReference<>(
+        ImmutableMap.<String, List<Rule>>of()
     );
   }
 
@@ -188,7 +187,7 @@ public class DatabaseRuleManager
         return;
       }
 
-      rules.set(new ConcurrentHashMap<String, List<Rule>>());
+      rules.set(ImmutableMap.<String, List<Rule>>of());
 
       started = false;
       exec.shutdownNow();
@@ -199,7 +198,7 @@ public class DatabaseRuleManager
   public void poll()
   {
     try {
-      ConcurrentHashMap<String, List<Rule>> newRules = new ConcurrentHashMap<String, List<Rule>>(
+      ImmutableMap<String, List<Rule>> newRules = ImmutableMap.copyOf(
           dbi.withHandle(
               new HandleCallback<Map<String, List<Rule>>>()
               {
@@ -309,12 +308,6 @@ public class DatabaseRuleManager
               }
             }
         );
-
-        ConcurrentHashMap<String, List<Rule>> existingRules = rules.get();
-        if (existingRules == null) {
-          existingRules = new ConcurrentHashMap<String, List<Rule>>();
-        }
-        existingRules.put(dataSource, newRules);
       }
       catch (Exception e) {
         log.error(e, String.format("Exception while overriding rule for %s", dataSource));
diff --git a/server/src/main/java/io/druid/server/coordinator/DruidCoordinator.java b/server/src/main/java/io/druid/server/coordinator/DruidCoordinator.java
index 8aa0f88e5db..f57bd87ebd5 100644
--- a/server/src/main/java/io/druid/server/coordinator/DruidCoordinator.java
+++ b/server/src/main/java/io/druid/server/coordinator/DruidCoordinator.java
@@ -350,6 +350,10 @@ public class DruidCoordinator
   )
   {
     try {
+      if (fromServer.getMetadata().equals(toServer.getMetadata())) {
+        throw new IAE("Cannot move [%s] to and from the same server [%s]", segmentName, fromServer.getName());
+      }
+
       final DataSegment segment = fromServer.getSegment(segmentName);
       if (segment == null) {
         throw new IAE("Unable to find segment [%s] on server [%s]", segmentName, fromServer.getName());
diff --git a/server/src/main/java/io/druid/server/coordinator/helper/DruidCoordinatorRuleRunner.java b/server/src/main/java/io/druid/server/coordinator/helper/DruidCoordinatorRuleRunner.java
index 15edf27d428..e0cb457418b 100644
--- a/server/src/main/java/io/druid/server/coordinator/helper/DruidCoordinatorRuleRunner.java
+++ b/server/src/main/java/io/druid/server/coordinator/helper/DruidCoordinatorRuleRunner.java
@@ -76,8 +76,8 @@ public class DruidCoordinatorRuleRunner implements DruidCoordinatorHelper
     }
 
     DruidCoordinatorRuntimeParams paramsWithReplicationManager = params.buildFromExisting()
-                                                                  .withReplicationManager(replicatorThrottler)
-                                                                  .build();
+                                                                       .withReplicationManager(replicatorThrottler)
+                                                                       .build();
 
     // Run through all matched rules for available segments
     DateTime now = new DateTime();
@@ -94,10 +94,8 @@ public class DruidCoordinatorRuleRunner implements DruidCoordinatorHelper
       }
 
       if (!foundMatchingRule) {
-        log.makeAlert(
-            "Unable to find a matching rule for dataSource[%s]",
-            segment.getDataSource()
-        )
+        log.makeAlert("Unable to find a matching rule!")
+           .addData("dataSource", segment.getDataSource())
            .addData("segment", segment.getIdentifier())
            .emit();
       }