hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@apache.org
Subject svn commit: r987974 - in /hbase/branches/0.90_master_rewrite: ./ src/main/java/org/apache/hadoop/hbase/client/ src/main/java/org/apache/hadoop/hbase/executor/ src/main/java/org/apache/hadoop/hbase/master/ src/main/java/org/apache/hadoop/hbase/master/ha...
Date Mon, 23 Aug 2010 01:40:53 GMT
Author: stack
Date: Mon Aug 23 01:40:52 2010
New Revision: 987974

URL: http://svn.apache.org/viewvc?rev=987974&view=rev
Log:

Making table schema modification, add/delete work.

M BRANCH_TODO.txt
  Added notes on sync vs async.

M src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
  Added more tests for add/delete column and table.
M src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
  Added utility method that implementors can just noop.
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
  execute removed.  Call process in stead.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
  Removed redundant table checks.  These checks have been moved into
  handler constructor.
  (checkTableModifiable) Added
M src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
M src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
  Renaming variables and allow throwing exception on construction
M src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
M src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
M src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
  Move table checks into constructor rather than in processor.
M src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
  Constructor throws IOE
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
  Updated javadoc.  Moved all to do with Executor out of here and into
  that class instead.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
  Javadoc and moved executor types here from EventHandler
M src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
  Make the table schema mod methods all work the same.

Modified:
    hbase/branches/0.90_master_rewrite/BRANCH_TODO.txt
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
    hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
    hbase/branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java

Modified: hbase/branches/0.90_master_rewrite/BRANCH_TODO.txt
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/BRANCH_TODO.txt?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/BRANCH_TODO.txt (original)
+++ hbase/branches/0.90_master_rewrite/BRANCH_TODO.txt Mon Aug 23 01:40:52 2010
@@ -8,8 +8,15 @@ remaining tasks before merge
 * finish baseline implementation of new splits
 
 * integrate load balancer
+- Looksee if we are still deleting location from meta; not needed any
+more and if we don't delete, then we can put region back on the server
+that used to be serving it; can add old location to new RegionPlan
+-- St.Ack 08/21
 
 * ensure root/meta are last to close on cluster shutdown
+- Add asking RS what it has when only two servers remaining...
+and when only root or meta, then send explicit close of each.
+Do it this way to ensure correct shutdown order -- St.Ack 08/21
 
 
 ---
@@ -120,6 +127,20 @@ somewhat easier stuff
 St.Ack
  -- Ensure root and meta are last to close on cluster shutdown; it shoudl be the case but
verify.
 
+From Master:
+
+  // TODO: Sync or async on this stuff? execute means sync.  submit means later.
+  //       Right now this will swallow exceptions either way, might need
+  //       process() which throws nothing but execute() which throws IOE so
+  //       synchronous stuff can throw exceptions?
+
+At the moment we have a mix of sync and async.  Whats missing is a callback mechanism
+(Benoit's Twisted Deferred would do nicely here).  I change EventHandler to
+remove the execute so synchronous call process directly.  Also made it so
+process now throws an exception  Also, changed handlers so they do checks
+in constructor and constructor throws IOE.  This way, we fail fast so even if
+asynchronous operation, its possible we'll see the TableNotDisabledException.
+There is more to do in here but should be good enough for merge.
  
  
 ================================================================================
@@ -254,4 +275,4 @@ Later:
     (if finish, need to use files not directory, and use right location)
   - Put notes from reviewboard/jira into LB javadoc or hbase "book"
 
-  
\ No newline at end of file
+  

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
Mon Aug 23 01:40:52 2010
@@ -594,7 +594,6 @@ public class HBaseAdmin {
    */
   public void deleteColumn(final byte [] tableName, final byte [] columnName)
   throws IOException {
-    HTableDescriptor.isLegalTableName(tableName);
     try {
       getMaster().deleteColumn(tableName, columnName);
     } catch (RemoteException e) {
@@ -657,9 +656,14 @@ public class HBaseAdmin {
    */
   public void modifyColumn(final byte [] tableName, HColumnDescriptor descriptor)
   throws IOException {
-    HTableDescriptor.isLegalTableName(tableName);
-    if (isTableEnabled(tableName)) throw new TableNotDisabledException(tableName);
-    getMaster().modifyColumn(tableName, descriptor);
+    try {
+      getMaster().modifyColumn(tableName, descriptor);
+    } catch (RemoteException re) {
+      // Convert RE exceptions in here; client shouldn't have to deal with them,
+      // at least w/ the type of exceptions that come out of this method:
+      // TableNotFoundException, etc.
+      throw RemoteExceptionHandler.decodeRemoteException(re);
+    }
   }
 
   /**
@@ -836,9 +840,14 @@ public class HBaseAdmin {
    */
   public void modifyTable(final byte [] tableName, HTableDescriptor htd)
   throws IOException {
-    HTableDescriptor.isLegalTableName(tableName);
-    if (isTableEnabled(tableName)) throw new TableNotDisabledException(tableName);
-    getMaster().modifyTable(tableName, htd);
+    try {
+      getMaster().modifyTable(tableName, htd);
+    } catch (RemoteException re) {
+      // Convert RE exceptions in here; client shouldn't have to deal with them,
+      // at least w/ the type of exceptions that come out of this method:
+      // TableNotFoundException, etc.
+      throw RemoteExceptionHandler.decodeRemoteException(re);
+    }
   }
 
 

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Mon Aug 23 01:40:52 2010
@@ -19,62 +19,83 @@
  */
 package org.apache.hadoop.hbase.executor;
 
+import java.io.IOException;
 import java.util.concurrent.atomic.AtomicLong;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.hbase.Server;
-import org.apache.hadoop.hbase.executor.ExecutorService.ExecutorType;
 
 
 /**
  * Abstract base class for all HBase event handlers. Subclasses should
- * implement the process() method where the actual handling of the event
- * happens.
+ * implement the {@link #process()} method.  Subclasses should also do all
+ * necessary checks up in their constructor if possible -- check table exists,
+ * is disabled, etc. -- so they fail fast rather than later when process is
+ * running.  Do it this way because process be invoked directly but event
+ * handlers are also
+ * run in an executor context -- i.e. asynchronously -- and in this case,
+ * exceptions thrown at process time will not be seen by the invoker, not till
+ * we implement a call-back mechanism so the client can pick them up later.
  * <p>
- * EventType is a list of ALL events (which also corresponds to messages -
- * either internal to one component or between components). The event type
- * names specify the component from which the event originated, and the
- * component which is supposed to handle it.
+ * Event handlers have an {@link EventType}.
+ * {@link EventType} is a list of ALL handler event types.  We need to keep
+ * a full list in one place -- and as enums is a good shorthand for an
+ * implemenations -- because event handlers can be passed to executors when
+ * they are to be run asynchronously. The
+ * hbase executor, see {@link ExecutorService}, has a switch for passing
+ * event type to executor.
  * <p>
- * Listeners can listen to all the events by implementing the interface
- * EventHandlerListener, and by registering themselves as a listener. They
- * will be called back before and after the process of every event.
+ * Event listeners can be installed and will be called pre- and post- process if
+ * this EventHandler is run in a Thread (its a Runnable so if its {@link #run()}
+ * method gets called).  Implement
+ * {@link EventHandlerListener}s, and registering using
+ * {@link #setListener(EventHandlerListener)}.
+ * @see {@link ExecutorService}
  */
 public abstract class EventHandler implements Runnable, Comparable<Runnable> {
   private static final Log LOG = LogFactory.getLog(EventHandler.class);
+
   // type of event this object represents
   protected EventType eventType;
-  // server controller
+
   protected Server server;
 
   // sequence id generator for default FIFO ordering of events
   protected static AtomicLong seqids = new AtomicLong(0);
+
   // sequence id for this event
-  protected long seqid;
+  private final long seqid;
 
-  // Listener to call pre- and post- processing.
+  // Listener to call pre- and post- processing.  May be null.
   private EventHandlerListener listener;
 
   /**
-   * This interface provides hooks to listen to various events received by the
-   * queue. A class implementing this can listen to the updates by calling
-   * registerListener and stop receiving updates by calling unregisterListener
+   * This interface provides pre- and post-process hooks for events.
    */
   public interface EventHandlerListener {
     /**
      * Called before any event is processed
+     * @param The event handler whose process method is about to be called.
      */
     public void beforeProcess(EventHandler event);
     /**
      * Called after any event is processed
+     * @param The event handler whose process method is about to be called.
      */
     public void afterProcess(EventHandler event);
   }
 
   /**
-   * These are a list of HBase events that can be handled by the various
-   * HBaseExecutorService's. All the events are serialized as byte values.
+   * List of all HBase event handler types.  Event types are named by a
+   * convention: event type names specify the component from which the event
+   * originated and then where its destined -- e.g. RS2ZK_ prefix means the
+   * event came from a regionserver destined for zookeeper -- and then what
+   * the even is; e.g. REGION_OPENING.
+   * 
+   * <p>We give the enums indices so we can add types later and keep them
+   * grouped together rather than have to add them always to the end as we
+   * would have to if we used raw enum ordinals.
    */
   public enum EventType {
     // Messages originating from RS (NOTE: there is NO direct communication from
@@ -109,71 +130,9 @@ public abstract class EventHandler imple
     M_SERVER_SHUTDOWN         (70);  // Master is processing shutdown of a RS
 
     /**
-     * Returns the executor service type (the thread pool instance) for this
-     * event type.  Every type must be handled here.  Multiple types map to
-     * Called by the HMaster. Returns a name of the executor service given an
-     * event type. Every event type has an entry - if the event should not be
-     * handled just add the NONE executor.
-     * @return name of the executor service
+     * Constructor
      */
-    public ExecutorType getExecutorServiceType() {
-      switch(this) {
-
-        // Master executor services
-
-        case RS2ZK_REGION_CLOSED:
-          return ExecutorType.MASTER_CLOSE_REGION;
-
-        case RS2ZK_REGION_OPENED:
-          return ExecutorType.MASTER_OPEN_REGION;
-
-        case M_SERVER_SHUTDOWN:
-          return ExecutorType.MASTER_SERVER_OPERATIONS;
-
-        case C2M_DELETE_TABLE:
-        case C2M_DISABLE_TABLE:
-        case C2M_ENABLE_TABLE:
-        case C2M_MODIFY_TABLE:
-          return ExecutorType.MASTER_TABLE_OPERATIONS;
-
-        // RegionServer executor services
-
-        case M2RS_OPEN_REGION:
-          return ExecutorType.RS_OPEN_REGION;
-
-        case M2RS_OPEN_ROOT:
-          return ExecutorType.RS_OPEN_ROOT;
-
-        case M2RS_OPEN_META:
-          return ExecutorType.RS_OPEN_META;
-
-        case M2RS_CLOSE_REGION:
-          return ExecutorType.RS_CLOSE_REGION;
-
-        case M2RS_CLOSE_ROOT:
-          return ExecutorType.RS_CLOSE_ROOT;
-
-        case M2RS_CLOSE_META:
-          return ExecutorType.RS_CLOSE_META;
-
-        default:
-          throw new RuntimeException("Unhandled event type " + this.name());
-      }
-    }
-
     EventType(int value) {}
-
-    @Override
-    public String toString() {
-      switch(this) {
-        case RS2ZK_REGION_CLOSED:   return "CLOSED";
-        case RS2ZK_REGION_CLOSING:  return "CLOSING";
-        case RS2ZK_REGION_OPENED:   return "OPENED";
-        case RS2ZK_REGION_OPENING:  return "OPENING";
-        case M2ZK_REGION_OFFLINE:   return "OFFLINE";
-        default:                    return this.name();
-      }
-    }
   }
 
   /**
@@ -185,40 +144,29 @@ public abstract class EventHandler imple
     seqid = seqids.incrementAndGet();
   }
 
-  /**
-   * This is a wrapper around {@link #process()} to give listeners a chance to run.
-   */
   public void run() {
-    if (getListener() != null) this.listener.beforeProcess(this);
-    // call the main process function
     try {
+      if (getListener() != null) getListener().beforeProcess(this);
       process();
+      if (getListener() != null) getListener().afterProcess(this);
     } catch(Throwable t) {
       LOG.error("Caught throwable while processing event " + eventType, t);
     }
-    if (getListener() != null) this.listener.afterProcess(this);
   }
 
   /**
    * This method is the main processing loop to be implemented by the various
    * subclasses.
+   * @throws IOException
    */
-  public abstract void process();
-
-  /**
-   * Return the name for this event type.
-   * @return
-   */
-  public ExecutorType getExecutorType() {
-    return eventType.getExecutorServiceType();
-  }
+  public abstract void process() throws IOException;
 
   /**
    * Return the event type
    * @return
    */
   public EventType getEventType() {
-    return eventType;
+    return this.eventType;
   }
 
   /**
@@ -238,6 +186,13 @@ public abstract class EventHandler imple
   }
 
   /**
+   * @return This events' sequence id.
+   */
+  public long getSeqid() {
+    return this.seqid;
+  }
+
+  /**
    * Default prioritized runnable comparator which implements a FIFO ordering.
    * <p>
    * Subclasses should not override this.  Instead, if they want to implement
@@ -253,14 +208,6 @@ public abstract class EventHandler imple
   }
 
   /**
-   * Executes this event object in the caller's thread. This is a synchronous
-   * way of executing the event.
-   */
-  public void execute() {
-    this.run();
-  }
-
-  /**
    * @return Current listener or null if none set.
    */
   public synchronized EventHandlerListener getListener() {

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Mon Aug 23 01:40:52 2010
@@ -45,9 +45,8 @@ import com.google.common.util.concurrent
  * then do: <code>instance.startExecutorService("myService");</code>.  When done
  * call {@link #shutdown()}.
  *
- * In order to use the service created above, you need to override the
- * {@link EventHandler} class and create an {@link EventHandler.EventType} that
- * submits to this service.  Register pre- and post- processing listeners
+ * <p>In order to use the service created above, call
+ * {@link #submit(EventHandler)}. Register pre- and post- processing listeners
  * by registering your implementation of {@link EventHandler.EventHandlerListener}
  * with {@link #registerListener(EventType, EventHandlerListener)}.  Be sure
  * to deregister your listener when done via {@link #unregisterListener(EventType)}.
@@ -58,15 +57,17 @@ public class ExecutorService {
   // hold the all the executors created in a map addressable by their names
   private final ConcurrentHashMap<String, Executor> executorMap =
     new ConcurrentHashMap<String, Executor>();
+
   // listeners that are called before and after an event is processed
   private ConcurrentHashMap<EventHandler.EventType, EventHandlerListener> eventHandlerListeners
=
     new ConcurrentHashMap<EventHandler.EventType, EventHandlerListener>();
 
+  // Name of the server hosting this executor service.
   private final String servername;
 
   /**
-   * The following is a list of names for the various executor services in both
-   * the master and the region server.
+   * The following is a list of all executor types, both those that run in the
+   * master and those that run in the regionserver.
    */
   public enum ExecutorType {
 
@@ -87,12 +88,65 @@ public class ExecutorService {
 
     ExecutorType(int value) {}
 
+    /**
+     * @param serverName
+     * @return Conflation of the executor type and the passed servername.
+     */
     String getExecutorName(String serverName) {
       return this.toString() + "-" + serverName;
     }
   }
 
   /**
+   * Returns the executor service type (the thread pool instance) for the
+   * passed event handler type.
+   * @param type EventHandler type.
+   */
+  public ExecutorType getExecutorServiceType(final EventHandler.EventType type) {
+    switch(type) {
+      // Master executor services
+
+      case RS2ZK_REGION_CLOSED:
+        return ExecutorType.MASTER_CLOSE_REGION;
+
+      case RS2ZK_REGION_OPENED:
+        return ExecutorType.MASTER_OPEN_REGION;
+
+      case M_SERVER_SHUTDOWN:
+        return ExecutorType.MASTER_SERVER_OPERATIONS;
+
+      case C2M_DELETE_TABLE:
+      case C2M_DISABLE_TABLE:
+      case C2M_ENABLE_TABLE:
+      case C2M_MODIFY_TABLE:
+        return ExecutorType.MASTER_TABLE_OPERATIONS;
+
+      // RegionServer executor services
+
+      case M2RS_OPEN_REGION:
+        return ExecutorType.RS_OPEN_REGION;
+
+      case M2RS_OPEN_ROOT:
+        return ExecutorType.RS_OPEN_ROOT;
+
+      case M2RS_OPEN_META:
+        return ExecutorType.RS_OPEN_META;
+
+      case M2RS_CLOSE_REGION:
+        return ExecutorType.RS_CLOSE_REGION;
+
+      case M2RS_CLOSE_ROOT:
+        return ExecutorType.RS_CLOSE_ROOT;
+
+      case M2RS_CLOSE_META:
+        return ExecutorType.RS_CLOSE_META;
+
+      default:
+        throw new RuntimeException("Unhandled event type " + type);
+    }
+  }
+
+  /**
    * Default constructor.
    * @param Name of the hosting server.
    */
@@ -158,7 +212,7 @@ public class ExecutorService {
   }
 
   public void submit(final EventHandler eh) {
-    getExecutor(eh.getExecutorType()).submit(eh);
+    getExecutor(getExecutorServiceType(eh.getEventType())).submit(eh);
   }
 
   /**

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Mon Aug 23 01:40:52 2010
@@ -197,7 +197,7 @@ public class AssignmentManager extends Z
           regionsInTransition.put(encodedName,
               new RegionState(regionInfo, RegionState.State.CLOSED,
                   data.getStamp()));
-          new ClosedRegionHandler(master, this, data, regionInfo).execute();
+          new ClosedRegionHandler(master, this, data, regionInfo).process();
           break;
 
         case RS2ZK_REGION_OPENING:
@@ -214,7 +214,7 @@ public class AssignmentManager extends Z
               new RegionState(regionInfo, RegionState.State.OPENING,
                   data.getStamp()));
           new OpenedRegionHandler(master, this, data, regionInfo,
-              serverManager.getServerInfo(data.getServerName())).execute();
+              serverManager.getServerInfo(data.getServerName())).process();
           break;
       }
     }
@@ -239,8 +239,7 @@ public class AssignmentManager extends Z
       }
       String encodedName = HRegionInfo.encodeRegionName(data.getRegionName());
       String prettyPrintedRegionName = HRegionInfo.prettyPrint(encodedName);
-      LOG.debug("Handling transition=" + data.getEventType().name() +
-        "/" + data.getEventType().toString() +
+      LOG.debug("Handling transition=" + data.getEventType() +
         ", server=" + data.getServerName() + ", region=" + prettyPrintedRegionName);
       RegionState regionState = regionsInTransition.get(encodedName);
       switch(data.getEventType()) {

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Mon Aug 23 01:40:52 2010
@@ -46,6 +46,8 @@ import org.apache.hadoop.hbase.NotAllMet
 import org.apache.hadoop.hbase.RemoteExceptionHandler;
 import org.apache.hadoop.hbase.Server;
 import org.apache.hadoop.hbase.TableExistsException;
+import org.apache.hadoop.hbase.TableNotDisabledException;
+import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.hadoop.hbase.ZooKeeperConnectionException;
 import org.apache.hadoop.hbase.catalog.CatalogTracker;
 import org.apache.hadoop.hbase.catalog.MetaEditor;
@@ -579,64 +581,38 @@ implements HMasterInterface, HMasterRegi
     }
   }
 
-  private boolean isCatalogTable(final byte [] tableName) {
+  private static boolean isCatalogTable(final byte [] tableName) {
     return Bytes.equals(tableName, HConstants.ROOT_TABLE_NAME) ||
            Bytes.equals(tableName, HConstants.META_TABLE_NAME);
   }
 
-  // TODO: Sync or async on this stuff?
-  //       Right now this will swallow exceptions either way, might need
-  //       process() which throws nothing but execute() which throws IOE so
-  //       synchronous stuff can throw exceptions?
-
   public void deleteTable(final byte [] tableName) throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't delete catalog tables");
-    }
-    //
-    new DeleteTableHandler(tableName, this, this)
-    .execute();
-    LOG.info("deleted table: " + Bytes.toString(tableName));
+    new DeleteTableHandler(tableName, this, this).process();
   }
 
   public void addColumn(byte [] tableName, HColumnDescriptor column)
   throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't modify catalog tables");
-    }
-    new TableAddFamilyHandler(tableName, column, this, this).execute();
+    new TableAddFamilyHandler(tableName, column, this, this).process();
   }
 
   public void modifyColumn(byte [] tableName, HColumnDescriptor descriptor)
   throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't modify catalog tables");
-    }
-    new TableModifyFamilyHandler(tableName, descriptor, this, this).execute();
+    new TableModifyFamilyHandler(tableName, descriptor, this, this).process();
   }
 
   public void deleteColumn(final byte [] tableName, final byte [] c)
   throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't modify catalog tables");
-    }
-    new TableDeleteFamilyHandler(tableName, c, this, this).execute();
+    new TableDeleteFamilyHandler(tableName, c, this, this).process();
   }
 
   public void enableTable(final byte [] tableName) throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't enable catalog tables");
-    }
     new EnableTableHandler(this, tableName, catalogTracker, assignmentManager)
-    .execute();
+      .process();
   }
 
   public void disableTable(final byte [] tableName) throws IOException {
-    if (isCatalogTable(tableName)) {
-      throw new IOException("Can't disable catalog tables");
-    }
     new DisableTableHandler(this, tableName, catalogTracker, assignmentManager)
-    .execute();
+      .process();
   }
 
   /**
@@ -682,6 +658,21 @@ implements HMasterInterface, HMasterRegi
     this.executorService.submit(new ModifyTableHandler(tableName, htd, this, this));
   }
 
+  @Override
+  public void checkTableModifiable(final byte [] tableName)
+  throws IOException {
+    String tableNameStr = Bytes.toString(tableName);
+    if (isCatalogTable(tableName)) {
+      throw new IOException("Can't modify catalog tables");
+    }
+    if (!MetaReader.tableExists(getCatalogTracker(), tableNameStr)) {
+      throw new TableNotFoundException(tableNameStr);
+    }
+    if (!getAssignmentManager().isTableDisabled(Bytes.toString(tableName))) {
+      throw new TableNotDisabledException(tableName);
+    }
+  }
+
   /**
    * @return cluster status
    */

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java
Mon Aug 23 01:40:52 2010
@@ -19,6 +19,8 @@
  */
 package org.apache.hadoop.hbase.master;
 
+import java.io.IOException;
+
 import org.apache.hadoop.hbase.catalog.CatalogTracker;
 import org.apache.hadoop.hbase.executor.ExecutorService;
 
@@ -50,4 +52,12 @@ public interface MasterServices {
    * @return Master's instance of {@link ExecutorService}
    */
   public ExecutorService getExecutorService();
+
+  /**
+   * Check table is modifiable; i.e. exists and is offline.
+   * @param tableName Name of table to check.
+   * @throws TableNotDisabledException
+   * @throws TableNotFoundException 
+   */
+  public void checkTableModifiable(final byte [] tableName) throws IOException;
 }

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
Mon Aug 23 01:40:52 2010
@@ -33,7 +33,7 @@ public class DeleteTableHandler extends 
   private static final Log LOG = LogFactory.getLog(DeleteTableHandler.class);
 
   public DeleteTableHandler(byte [] tableName, Server server,
-      final MasterServices masterServices) {
+      final MasterServices masterServices) throws IOException {
     super(EventType.C2M_DELETE_TABLE, tableName, server, masterServices);
   }
 

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
Mon Aug 23 01:40:52 2010
@@ -33,20 +33,20 @@ public class ModifyTableHandler extends 
 
   public ModifyTableHandler(final byte [] tableName,
       final HTableDescriptor htd, final Server server,
-      final MasterServices masterServices) {
+      final MasterServices masterServices) throws IOException {
     super(EventType.C2M_MODIFY_TABLE, tableName, server, masterServices);
     this.htd = htd;
   }
 
   @Override
-  protected void handleTableOperation(List<HRegionInfo> regions)
+  protected void handleTableOperation(List<HRegionInfo> hris)
   throws IOException {
-    for (HRegionInfo region : regions) {
+    for (HRegionInfo hri : hris) {
       // Update region info in META
-      region.setTableDesc(this.htd);
-      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), region);
+      hri.setTableDesc(this.htd);
+      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), hri);
       // Update region info in FS
-      this.masterServices.getMasterFileSystem().updateRegionInfo(region);
+      this.masterServices.getMasterFileSystem().updateRegionInfo(hri);
     }
   }
 }
\ No newline at end of file

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
Mon Aug 23 01:40:52 2010
@@ -39,28 +39,28 @@ public class TableAddFamilyHandler exten
   private final HColumnDescriptor familyDesc;
 
   public TableAddFamilyHandler(byte[] tableName, HColumnDescriptor familyDesc,
-      Server server, final MasterServices masterServices) {
+      Server server, final MasterServices masterServices) throws IOException {
     super(EventType.C2M_ADD_FAMILY, tableName, server, masterServices);
     this.familyDesc = familyDesc;
   }
 
   @Override
-  protected void handleTableOperation(List<HRegionInfo> regions)
+  protected void handleTableOperation(List<HRegionInfo> hris)
   throws IOException {
-    HTableDescriptor htd = regions.get(0).getTableDesc();
+    HTableDescriptor htd = hris.get(0).getTableDesc();
     byte [] familyName = familyDesc.getName();
     if(htd.hasFamily(familyName)) {
       throw new InvalidFamilyOperationException(
           "Family '" + Bytes.toString(familyName) + "' already exists so " +
           "cannot be added");
     }
-    for(HRegionInfo region : regions) {
+    for(HRegionInfo hri : hris) {
       // Update the HTD
-      region.getTableDesc().addFamily(familyDesc);
+      hri.getTableDesc().addFamily(familyDesc);
       // Update region in META
-      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), region);
+      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), hri);
       // Update region info in FS
-      this.masterServices.getMasterFileSystem().updateRegionInfo(region);
+      this.masterServices.getMasterFileSystem().updateRegionInfo(hri);
     }
   }
 }
\ No newline at end of file

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
Mon Aug 23 01:40:52 2010
@@ -39,29 +39,31 @@ public class TableDeleteFamilyHandler ex
   private final byte [] familyName;
 
   public TableDeleteFamilyHandler(byte[] tableName, byte [] familyName,
-      Server server, final MasterServices masterServices) {
+      Server server, final MasterServices masterServices) throws IOException {
     super(EventType.C2M_ADD_FAMILY, tableName, server, masterServices);
     this.familyName = familyName;
   }
 
   @Override
-  protected void handleTableOperation(List<HRegionInfo> regions) throws IOException
{
-    HTableDescriptor htd = regions.get(0).getTableDesc();
+  protected void handleTableOperation(List<HRegionInfo> hris) throws IOException {
+    HTableDescriptor htd = hris.get(0).getTableDesc();
     if(!htd.hasFamily(familyName)) {
       throw new InvalidFamilyOperationException(
           "Family '" + Bytes.toString(familyName) + "' does not exist so " +
           "cannot be deleted");
     }
-    for(HRegionInfo region : regions) {
+    for(HRegionInfo hri : hris) {
       // Update the HTD
-      region.getTableDesc().removeFamily(familyName);
+      hri.getTableDesc().removeFamily(familyName);
       // Update region in META
-      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), region);
+      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), hri);
       MasterFileSystem mfs = this.masterServices.getMasterFileSystem();
       // Update region info in FS
-      mfs.updateRegionInfo(region);
+      mfs.updateRegionInfo(hri);
       // Delete directory in FS
-      mfs.deleteFamily(region, familyName);
+      mfs.deleteFamily(hri, familyName);
+      // Update region info in FS
+      this.masterServices.getMasterFileSystem().updateRegionInfo(hri);
     }
   }
-}
+}
\ No newline at end of file

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
Mon Aug 23 01:40:52 2010
@@ -26,8 +26,6 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.Server;
-import org.apache.hadoop.hbase.TableNotDisabledException;
-import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.hadoop.hbase.catalog.MetaReader;
 import org.apache.hadoop.hbase.executor.EventHandler;
 import org.apache.hadoop.hbase.master.MasterServices;
@@ -35,23 +33,23 @@ import org.apache.hadoop.hbase.util.Byte
 
 /**
  * Base class for performing operations against tables.
- * <p>
- * Ensures all regions of the table are offline and then executes
- * {@link #handleTableOperation(List)} with a list of regions of the
- * table.
+ * Checks on whether the process can go forward are done in constructor rather
+ * than later on in {@link #process()}.  The idea is to fail fast rather than
+ * later down in an async invocation of {@link #process()} (which currently has
+ * no means of reporting back issues once started).
  */
 public abstract class TableEventHandler extends EventHandler {
   private static final Log LOG = LogFactory.getLog(TableEventHandler.class);
   protected final MasterServices masterServices;
   protected final byte [] tableName;
-  protected final String tableNameStr;
 
   public TableEventHandler(EventType eventType, byte [] tableName, Server server,
-      MasterServices masterServices) {
+      MasterServices masterServices)
+  throws IOException {
     super(server, eventType);
     this.masterServices = masterServices;
     this.tableName = tableName;
-    this.tableNameStr = Bytes.toString(this.tableName);
+    this.masterServices.checkTableModifiable(tableName);
   }
 
   @Override
@@ -59,30 +57,16 @@ public abstract class TableEventHandler 
     try {
       LOG.info("Handling table operation " + eventType + " on table " +
           Bytes.toString(tableName));
-      handleTableOperation(tableChecks());
+      List<HRegionInfo> hris =
+        MetaReader.getTableRegions(this.masterServices.getCatalogTracker(),
+          tableName);
+      handleTableOperation(hris);
     } catch (IOException e) {
       LOG.error("Error trying to delete the table " + Bytes.toString(tableName),
           e);
     }
   }
 
-  private List<HRegionInfo> tableChecks() throws IOException {
-    // Check if table exists
-    if (!MetaReader.tableExists(this.masterServices.getCatalogTracker(),
-        this.tableNameStr)) {
-      throw new TableNotFoundException(Bytes.toString(tableName));
-    }
-    // Verify table is offline
-    if (!this.masterServices.getAssignmentManager().
-        isTableDisabled(this.tableNameStr)) {
-      throw new TableNotDisabledException(tableName);
-    }
-    // Get the regions of this table
-    // TODO: Use in-memory state of master?
-    return MetaReader.getTableRegions(this.masterServices.getCatalogTracker(),
-      tableName);
-  }
-
   protected abstract void handleTableOperation(List<HRegionInfo> regions)
   throws IOException;
 }
\ No newline at end of file

Modified: hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
Mon Aug 23 01:40:52 2010
@@ -40,7 +40,7 @@ public class TableModifyFamilyHandler ex
 
   public TableModifyFamilyHandler(byte[] tableName,
       HColumnDescriptor familyDesc, Server server,
-      final MasterServices masterServices) {
+      final MasterServices masterServices) throws IOException {
     super(EventType.C2M_MODIFY_FAMILY, tableName, server, masterServices);
     this.familyDesc = familyDesc;
   }
@@ -53,13 +53,13 @@ public class TableModifyFamilyHandler ex
       throw new InvalidFamilyOperationException("Family '" +
         Bytes.toString(familyName) + "' doesn't exists so cannot be modified");
     }
-    for(HRegionInfo region : regions) {
+    for(HRegionInfo hri : regions) {
       // Update the HTD
-      region.getTableDesc().addFamily(familyDesc);
+      hri.getTableDesc().addFamily(familyDesc);
       // Update region in META
-      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), region);
+      MetaEditor.updateRegionInfo(this.masterServices.getCatalogTracker(), hri);
       // Update region info in FS
-      this.masterServices.getMasterFileSystem().updateRegionInfo(region);
+      this.masterServices.getMasterFileSystem().updateRegionInfo(hri);
     }
   }
 }
\ No newline at end of file

Modified: hbase/branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java?rev=987974&r1=987973&r2=987974&view=diff
==============================================================================
--- hbase/branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
(original)
+++ hbase/branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
Mon Aug 23 01:40:52 2010
@@ -116,7 +116,7 @@ public class TestAdmin {
     boolean expectedException = false;
     try {
       this.admin.modifyTable(tableName, copy);
-    } catch (TableNotDisabledException e) {
+    } catch (TableNotDisabledException re) {
       expectedException = true;
     }
     assertTrue(expectedException);
@@ -130,7 +130,7 @@ public class TestAdmin {
     assertEquals(newFlushSize, modifiedHtd.getMemStoreFlushSize());
     assertEquals(key, modifiedHtd.getValue(key));
 
-    // Reenable table.
+    // Reenable table to test it fails if not disabled.
     this.admin.enableTable(tableName);
     assertFalse(this.admin.isTableDisabled(tableName));
 
@@ -141,25 +141,53 @@ public class TestAdmin {
     int maxversions = hcd.getMaxVersions();
     final int newMaxVersions = maxversions + 1;
     hcd.setMaxVersions(newMaxVersions);
-    expectedException = false;
     final byte [] hcdName = hcd.getName();
+    expectedException = false;
     try {
       this.admin.modifyColumn(tableName, hcd);
-      LOG.info("Modified column");
-    } catch (TableNotDisabledException e) {
-      LOG.error("EXCEP", e);
+    } catch (TableNotDisabledException re) {
       expectedException = true;
     }
     assertTrue(expectedException);
     this.admin.disableTable(tableName);
     assertTrue(this.admin.isTableDisabled(tableName));
-    modifyColumn(tableName, hcd);
+    // Modify Column is synchronous
     this.admin.modifyColumn(tableName, hcd);
     modifiedHtd = this.admin.getTableDescriptor(tableName);
     HColumnDescriptor modifiedHcd = modifiedHtd.getFamily(hcdName);
     assertEquals(newMaxVersions, modifiedHcd.getMaxVersions());
 
-    TODO: ADD/REMOVE COLUMN, REMOVE TABLE
+    // Try adding a column
+    // Reenable table to test it fails if not disabled.
+    this.admin.enableTable(tableName);
+    assertFalse(this.admin.isTableDisabled(tableName));
+    final String xtracolName = "xtracol";
+    HColumnDescriptor xtracol = new HColumnDescriptor(xtracolName);
+    xtracol.setValue(xtracolName, xtracolName);
+    try {
+      this.admin.addColumn(tableName, xtracol);
+    } catch (TableNotDisabledException re) {
+      expectedException = true;
+    }
+    assertTrue(expectedException);
+    this.admin.disableTable(tableName);
+    assertTrue(this.admin.isTableDisabled(tableName));
+    this.admin.addColumn(tableName, xtracol);
+    modifiedHtd = this.admin.getTableDescriptor(tableName);
+    hcd = modifiedHtd.getFamily(xtracol.getName());
+    assertTrue(hcd != null);
+    assertTrue(hcd.getValue(xtracolName).equals(xtracolName));
+
+    // Delete the just-added column.
+    this.admin.deleteColumn(tableName, xtracol.getName());
+    modifiedHtd = this.admin.getTableDescriptor(tableName);
+    hcd = modifiedHtd.getFamily(xtracol.getName());
+    assertTrue(hcd == null);
+
+    // Delete the table
+    this.admin.deleteTable(tableName);
+    this.admin.listTables();
+    assertFalse(this.admin.tableExists(tableName));
   }
 
   /**
@@ -188,30 +216,6 @@ public class TestAdmin {
   }
 
   /**
-   * Modify table is async so wait on completion of the table operation in master.
-   * @param tableName
-   * @param htd
-   * @throws IOException
-   */
-  private void modifyColumn(final byte [] tableName, final HColumnDescriptor hcd)
-  throws IOException {
-    MasterServices services = TEST_UTIL.getMiniHBaseCluster().getMaster();
-    ExecutorService executor = services.getExecutorService();
-    AtomicBoolean done = new AtomicBoolean(false);
-    executor.registerListener(EventType.C2M_MODIFY_FAMILY, new DoneListener(done));
-    this.admin.modifyColumn(tableName, hcd);
-    while (!done.get()) {
-      synchronized (done) {
-        try {
-          done.wait(1000);
-        } catch (InterruptedException e) {
-          e.printStackTrace();
-        }
-      }
-    }
-  }
-
-  /**
    * Listens for when an event is done in Master.
    */
   static class DoneListener implements EventHandler.EventHandlerListener {



Mime
View raw message