HDFS-13226. RBF: Throw the exception if mount table entry validated failed. Contributed by maobaolong.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/BaseRecord.java

@@ -32,6 +32,10 @@ import org.apache.hadoop.util.Time;
  * </ul>
  */
 public abstract class BaseRecord implements Comparable<BaseRecord> {
+  public static final String ERROR_MSG_CREATION_TIME_NEGATIVE =
+      "The creation time for the record cannot be negative.";
+  public static final String ERROR_MSG_MODIFICATION_TIME_NEGATIVE =
+      "The modification time for the record cannot be negative.";
 
   /**
    * Set the modification time for the record.
@@ -193,11 +197,15 @@ public abstract class BaseRecord implements Comparable<BaseRecord> {
 
   /**
    * Validates the record. Called when the record is created, populated from the
-   * state store, and before committing to the state store.
+   * state store, and before committing to the state store. If validate failed,
-   * @return If the record is valid.
+   * there throws an exception.
    */
-  public boolean validate() {
+  public void validate() {
-    return getDateCreated() > 0 && getDateModified() > 0;
+    if (getDateCreated() <= 0) {
+      throw new IllegalArgumentException(ERROR_MSG_CREATION_TIME_NEGATIVE);
+    } else if (getDateModified() <= 0) {
+      throw new IllegalArgumentException(ERROR_MSG_MODIFICATION_TIME_NEGATIVE);
+    }
   }
 
   @Override

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MembershipState.java

@@ -37,6 +37,14 @@ import org.apache.hadoop.hdfs.server.federation.store.driver.StateStoreSerialize
  */
 public abstract class MembershipState extends BaseRecord
     implements FederationNamenodeContext {
+  public static final String ERROR_MSG_NO_NS_SPECIFIED =
+      "Invalid registration, no nameservice specified ";
+  public static final String ERROR_MSG_NO_WEB_ADDR_SPECIFIED =
+      "Invalid registration, no web address specified ";
+  public static final String ERROR_MSG_NO_RPC_ADDR_SPECIFIED =
+      "Invalid registration, no rpc address specified ";
+  public static final String ERROR_MSG_NO_BP_SPECIFIED =
+      "Invalid registration, no block pool specified ";
 
   /** Expiration time in ms for this entry. */
   private static long expirationMs;
@@ -226,26 +234,25 @@ public abstract class MembershipState extends BaseRecord
    * is missing required information.
    */
   @Override
-  public boolean validate() {
+  public void validate() {
-    boolean ret = super.validate();
+    super.validate();
     if (getNameserviceId() == null || getNameserviceId().length() == 0) {
-      //LOG.error("Invalid registration, no nameservice specified " + this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_NS_SPECIFIED + this);
     }
     if (getWebAddress() == null || getWebAddress().length() == 0) {
-      //LOG.error("Invalid registration, no web address specified " + this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_WEB_ADDR_SPECIFIED + this);
     }
     if (getRpcAddress() == null || getRpcAddress().length() == 0) {
-      //LOG.error("Invalid registration, no rpc address specified " + this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_RPC_ADDR_SPECIFIED + this);
     }
     if (!isBadState() &&
         (getBlockPoolId().isEmpty() || getBlockPoolId().length() == 0)) {
-      //LOG.error("Invalid registration, no block pool specified " + this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_BP_SPECIFIED + this);
     }
-    return ret;
   }
 
 

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java

@@ -49,7 +49,18 @@ import org.slf4j.LoggerFactory;
 public abstract class MountTable extends BaseRecord {
 
   private static final Logger LOG = LoggerFactory.getLogger(MountTable.class);
-
+  public static final String ERROR_MSG_NO_SOURCE_PATH =
+      "Invalid entry, no source path specified ";
+  public static final String ERROR_MSG_MUST_START_WITH_BACK_SLASH =
+      "Invalid entry, all mount points must start with / ";
+  public static final String ERROR_MSG_NO_DEST_PATH_SPECIFIED =
+      "Invalid entry, no destination paths specified ";
+  public static final String ERROR_MSG_INVAILD_DEST_NS =
+      "Invalid entry, invalid destination nameservice ";
+  public static final String ERROR_MSG_INVAILD_DEST_PATH =
+      "Invalid entry, invalid destination path ";
+  public static final String ERROR_MSG_ALL_DEST_MUST_START_WITH_BACK_SLASH =
+      "Invalid entry, all destination must start with / ";
 
   /** Comparator for paths which considers the /. */
   public static final Comparator<String> PATH_COMPARATOR =
@@ -314,36 +325,35 @@ public abstract class MountTable extends BaseRecord {
   }
 
   @Override
-  public boolean validate() {
+  public void validate() {
-    boolean ret = super.validate();
+    super.validate();
     if (this.getSourcePath() == null || this.getSourcePath().length() == 0) {
-      LOG.error("Invalid entry, no source path specified ", this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_SOURCE_PATH + this);
     }
     if (!this.getSourcePath().startsWith("/")) {
-      LOG.error("Invalid entry, all mount points must start with / ", this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_MUST_START_WITH_BACK_SLASH + this);
     }
     if (this.getDestinations() == null || this.getDestinations().size() == 0) {
-      LOG.error("Invalid entry, no destination paths specified ", this);
+      throw new IllegalArgumentException(
-      ret = false;
+          ERROR_MSG_NO_DEST_PATH_SPECIFIED + this);
     }
     for (RemoteLocation loc : getDestinations()) {
       String nsId = loc.getNameserviceId();
       if (nsId == null || nsId.length() == 0) {
-        LOG.error("Invalid entry, invalid destination nameservice ", this);
+        throw new IllegalArgumentException(
-        ret = false;
+            ERROR_MSG_INVAILD_DEST_NS + this);
       }
       if (loc.getDest() == null || loc.getDest().length() == 0) {
-        LOG.error("Invalid entry, invalid destination path ", this);
+        throw new IllegalArgumentException(
-        ret = false;
+            ERROR_MSG_INVAILD_DEST_PATH + this);
       }
       if (!loc.getDest().startsWith("/")) {
-        LOG.error("Invalid entry, all destination must start with / ", this);
+        throw new IllegalArgumentException(
-        ret = false;
+            ERROR_MSG_ALL_DEST_MUST_START_WITH_BACK_SLASH + this);
       }
     }
-    return ret;
   }
 
   @Override

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/RouterState.java

@@ -127,14 +127,13 @@ public abstract class RouterState extends BaseRecord {
   }
 
   @Override
-  public boolean validate() {
+  public void validate() {
-    boolean ret = super.validate();
+    super.validate();
     if ((getAddress() == null || getAddress().length() == 0) &&
         getStatus() != RouterServiceState.INITIALIZING) {
-      LOG.error("Invalid router entry, no address specified {}", this);
+      throw new IllegalArgumentException(
-      ret = false;
+          "Invalid router entry, no address specified " + this);
     }
-    return ret;
   }
 
   @Override

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java

@@ -43,7 +43,6 @@ import org.apache.hadoop.test.GenericTestUtils;
 import org.apache.hadoop.util.ToolRunner;
 import org.junit.After;
 import org.junit.AfterClass;
-import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
@@ -109,7 +108,7 @@ public class TestRouterAdminCLI {
     String src = "/test-addmounttable";
     String dest = "/addmounttable";
     String[] argv = new String[] {"-add", src, nsId, dest};
-    Assert.assertEquals(0, ToolRunner.run(admin, argv));
+    assertEquals(0, ToolRunner.run(admin, argv));
 
     stateStore.loadCache(MountTableStoreImpl.class, true);
     GetMountTableEntriesRequest getRequest = GetMountTableEntriesRequest
@@ -129,7 +128,7 @@ public class TestRouterAdminCLI {
     // test mount table update behavior
     dest = dest + "-new";
     argv = new String[] {"-add", src, nsId, dest, "-readonly"};
-    Assert.assertEquals(0, ToolRunner.run(admin, argv));
+    assertEquals(0, ToolRunner.run(admin, argv));
     stateStore.loadCache(MountTableStoreImpl.class, true);
 
     getResponse = client.getMountTableManager()
@@ -211,7 +210,7 @@ public class TestRouterAdminCLI {
   @Test
   public void testMountTableDefaultACL() throws Exception {
     String[] argv = new String[] {"-add", "/testpath0", "ns0", "/testdir0"};
-    Assert.assertEquals(0, ToolRunner.run(admin, argv));
+    assertEquals(0, ToolRunner.run(admin, argv));
 
     stateStore.loadCache(MountTableStoreImpl.class, true);
     GetMountTableEntriesRequest getRequest = GetMountTableEntriesRequest
@@ -328,6 +327,37 @@ public class TestRouterAdminCLI {
     assertTrue(out.toString().contains("false"));
   }
 
+  @Test
+  public void testCreateInvalidEntry() throws Exception {
+    String[] argv = new String[] {
+        "-add", "test-createInvalidEntry", "ns0", "/createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", "/test-createInvalidEntry", "ns0", "createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", null, "ns0", "/createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", "/test-createInvalidEntry", "ns0", null};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", "", "ns0", "/createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", "/test-createInvalidEntry", null, "/createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+
+    argv = new String[] {
+        "-add", "/test-createInvalidEntry", "", "/createInvalidEntry"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+  }
+
   /**
    * Wait for the Router transforming to expected state.
    * @param expectedState Expected Router state.

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestMountTable.java

@@ -19,9 +19,12 @@ package org.apache.hadoop.hdfs.server.federation.store.records;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.IOException;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
@@ -30,6 +33,7 @@ import java.util.Map;
 import org.apache.hadoop.hdfs.server.federation.resolver.RemoteLocation;
 import org.apache.hadoop.hdfs.server.federation.resolver.order.DestinationOrder;
 import org.apache.hadoop.hdfs.server.federation.store.driver.StateStoreSerializer;
+import org.apache.hadoop.test.GenericTestUtils;
 import org.junit.Test;
 
 /**
@@ -172,4 +176,43 @@ public class TestMountTable {
     assertEquals(DST_NS_1, location2.getNameserviceId());
     assertEquals(DST_PATH_1, location2.getDest());
   }
+
+  @Test
+  public void testValidation() throws IOException {
+    Map<String, String> destinations = new HashMap<>();
+    destinations.put("ns0", "/testValidate-dest");
+    try {
+      MountTable.newInstance("testValidate", destinations);
+      fail("Mount table entry should be created failed.");
+    } catch (Exception e) {
+      GenericTestUtils.assertExceptionContains(
+          MountTable.ERROR_MSG_MUST_START_WITH_BACK_SLASH, e);
+    }
+
+    destinations.clear();
+    destinations.put("ns0", "testValidate-dest");
+    try {
+      MountTable.newInstance("/testValidate", destinations);
+      fail("Mount table entry should be created failed.");
+    } catch (Exception e) {
+      GenericTestUtils.assertExceptionContains(
+          MountTable.ERROR_MSG_ALL_DEST_MUST_START_WITH_BACK_SLASH, e);
+    }
+
+    destinations.clear();
+    destinations.put("", "/testValidate-dest");
+    try {
+      MountTable.newInstance("/testValidate", destinations);
+      fail("Mount table entry should be created failed.");
+    } catch (Exception e) {
+      GenericTestUtils.assertExceptionContains(
+          MountTable.ERROR_MSG_INVAILD_DEST_NS, e);
+    }
+
+    destinations.clear();
+    destinations.put("ns0", "/testValidate-dest");
+    MountTable record = MountTable.newInstance("/testValidate", destinations);
+    assertNotNull(record);
+
+  }
 }