accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bhava...@apache.org
Subject git commit: ACCUMULO-2345 Improve ConstraintCheck.check and add unit test
Date Tue, 25 Feb 2014 15:07:58 GMT
Repository: accumulo
Updated Branches:
  refs/heads/master 6bb1a1821 -> 5f726968e


ACCUMULO-2345 Improve ConstraintCheck.check and add unit test

Signed-off-by: Bill Havanki <bhavanki@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo
Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/5f726968
Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/5f726968
Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/5f726968

Branch: refs/heads/master
Commit: 5f726968ec19de8c37255656933509a9528b40ff
Parents: 6bb1a18
Author: Vikram Srivastava <vikrams@cloudera.com>
Authored: Mon Feb 10 21:28:15 2014 -0800
Committer: Bill Havanki <bhavanki@cloudera.com>
Committed: Tue Feb 25 09:59:18 2014 -0500

----------------------------------------------------------------------
 .../tserver/constraints/ConstraintChecker.java  |  67 +++++-----
 .../constraints/ConstraintCheckerTest.java      | 133 +++++++++++++++++++
 2 files changed, 169 insertions(+), 31 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/accumulo/blob/5f726968/server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/ConstraintChecker.java
----------------------------------------------------------------------
diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/ConstraintChecker.java
b/server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/ConstraintChecker.java
index 6f52485..7ea1388 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/ConstraintChecker.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/ConstraintChecker.java
@@ -33,6 +33,8 @@ import org.apache.accumulo.server.conf.TableConfiguration;
 import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader;
 import org.apache.log4j.Logger;
 
+import com.google.common.annotations.VisibleForTesting;
+
 public class ConstraintChecker {
   
   private ArrayList<Constraint> constrains;
@@ -75,7 +77,12 @@ public class ConstraintChecker {
       log.error("Failed to load constraints " + conf.getTableId() + " " + e.toString(), e);
     }
   }
-  
+
+  @VisibleForTesting
+  ArrayList<Constraint> getConstraints() {
+    return constrains;
+  }
+
   public boolean classLoaderChanged() {
     
     if (constrains.size() == 0)
@@ -99,12 +106,17 @@ public class ConstraintChecker {
     }
   }
 
+  private static Violations addViolation(Violations violations, ConstraintViolationSummary
cvs) {
+    if (violations == null) {
+      violations = new Violations();
+    }
+    violations.add(cvs);
+    return violations;
+  }
+
   public Violations check(Environment env, Mutation m) {
-    
-    Violations violations = null;
-    
     if (!env.getExtent().contains(new ComparableBytes(m.getRow()))) {
-      violations = new Violations();
+      Violations violations = new Violations();
       
       ConstraintViolationSummary cvs = new ConstraintViolationSummary(SystemConstraint.class.getName(),
(short) -1, "Mutation outside of tablet extent", 1);
       violations.add(cvs);
@@ -112,31 +124,26 @@ public class ConstraintChecker {
       // do not bother with further checks since this mutation does not go with this tablet
       return violations;
     }
-    
-    for (Constraint constraint : constrains) {
-      List<Short> violationCodes = null;
-      Throwable throwable = null;
-      
+
+    // violations is intentionally initialized as null for performance
+    Violations violations = null;
+    for (Constraint constraint : getConstraints()) {
       try {
-        violationCodes = constraint.check(env, m);
-      } catch (Throwable t) {
-        throwable = t;
-        log.warn("CONSTRAINT FAILED : " + throwable.getMessage(), t);
-      }
-      
-      if (violationCodes != null) {
-        for (Short vcode : violationCodes) {
-          ConstraintViolationSummary cvs = new ConstraintViolationSummary(constraint.getClass().getName(),
vcode, constraint.getViolationDescription(vcode), 1);
-          if (violations == null)
-            violations = new Violations();
-          violations.add(cvs);
+        List<Short> violationCodes = constraint.check(env, m);
+        if (violationCodes != null) {
+          String className = constraint.getClass().getName();
+          for (Short vcode : violationCodes) {
+            violations = addViolation(violations, new ConstraintViolationSummary(
+                className, vcode, constraint.getViolationDescription(vcode), 1));
+          }
         }
-      } else if (throwable != null) {
+      } catch (Throwable throwable) {
+        log.warn("CONSTRAINT FAILED : " + throwable.getMessage(), throwable);
+
         // constraint failed in some way, do not allow mutation to pass
-        
         short vcode;
         String msg;
-        
+
         if (throwable instanceof NullPointerException) {
           vcode = -1;
           msg = "threw NullPointerException";
@@ -153,14 +160,12 @@ public class ConstraintChecker {
           vcode = -100;
           msg = "threw some Exception";
         }
-        
-        ConstraintViolationSummary cvs = new ConstraintViolationSummary(constraint.getClass().getName(),
vcode, "CONSTRAINT FAILED : " + msg, 1);
-        if (violations == null)
-          violations = new Violations();
-        violations.add(cvs);
+
+        violations = addViolation(violations, new ConstraintViolationSummary(
+            constraint.getClass().getName(), vcode, "CONSTRAINT FAILED : " + msg, 1));
       }
     }
-    
+
     return violations;
   }
 }

http://git-wip-us.apache.org/repos/asf/accumulo/blob/5f726968/server/tserver/src/test/java/org/apache/accumulo/tserver/constraints/ConstraintCheckerTest.java
----------------------------------------------------------------------
diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/constraints/ConstraintCheckerTest.java
b/server/tserver/src/test/java/org/apache/accumulo/tserver/constraints/ConstraintCheckerTest.java
new file mode 100644
index 0000000..490c096
--- /dev/null
+++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/constraints/ConstraintCheckerTest.java
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.tserver.constraints;
+
+import static org.easymock.EasyMock.*;
+import static org.junit.Assert.*;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.core.constraints.Constraint;
+import org.apache.accumulo.core.constraints.Constraint.Environment;
+import org.apache.accumulo.core.data.ConstraintViolationSummary;
+import org.apache.accumulo.core.data.KeyExtent;
+import org.apache.accumulo.core.data.Mutation;
+import org.apache.hadoop.io.BinaryComparable;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+
+public class ConstraintCheckerTest {
+
+  private ConstraintChecker cc;
+  private ArrayList<Constraint> constraints;
+  private Environment env;
+  private KeyExtent extent;
+  private Mutation m;
+
+  @Before
+  public void setup() throws NoSuchMethodException, SecurityException {
+    cc = createMockBuilder(ConstraintChecker.class)
+           .addMockedMethod("getConstraints")
+           .createMock();
+    constraints = new ArrayList<Constraint>();
+    expect(cc.getConstraints()).andReturn(constraints);
+
+    env = createMock(Environment.class);
+    extent = createMock(KeyExtent.class);
+    expect(env.getExtent()).andReturn(extent);
+
+    m = createMock(Mutation.class);
+  }
+
+  private Constraint makeSuccessConstraint() {
+    Constraint c = createMock(Constraint.class);
+    expect(c.check(env, m)).andReturn(null); // no violations
+    replay(c);
+    return c;
+  }
+
+  private Constraint makeFailureConstraint() {
+    Constraint c = createMock(Constraint.class);
+    short code1 = 2;
+    short code2 = 4;
+    List<Short> vCodes = ImmutableList.of(code1, code2);
+    expect(c.getViolationDescription(code1)).andReturn("ViolationCode1");
+    expect(c.getViolationDescription(code2)).andReturn("ViolationCode2");
+    expect(c.check(env, m)).andReturn(vCodes);
+    replay(c);
+    return c;
+  }
+
+  private void replayAll() {
+    replay(extent);
+    replay(env);
+    replay(cc);
+  }
+
+  private Constraint makeExceptionConstraint() {
+    Constraint c = createMock(Constraint.class);
+    expect(c.check(env, m)).andThrow(new RuntimeException("some exception"));
+    replay(c);
+    return c;
+  }
+
+  @Test
+  public void testCheckAllOK() {
+    expect(extent.contains(anyObject(BinaryComparable.class))).andReturn(true);
+    replayAll();
+    constraints.add(makeSuccessConstraint());
+    assertNull(cc.check(env, m));
+  }
+
+  @Test
+  public void testCheckMutationOutsideKeyExtent() {
+    expect(extent.contains(anyObject(BinaryComparable.class))).andReturn(false);
+    replayAll();
+    ConstraintViolationSummary cvs = Iterables.getOnlyElement(cc.check(env, m).asList());
+    assertEquals(SystemConstraint.class.getName(), cvs.getConstrainClass());
+  }
+
+  @Test
+  public void testCheckFailure() {
+    expect(extent.contains(anyObject(BinaryComparable.class))).andReturn(true);
+    replayAll();
+    constraints.add(makeFailureConstraint());
+    List<ConstraintViolationSummary> cvsList = cc.check(env, m).asList();
+    assertEquals(2, cvsList.size());
+    Set<String> violationDescs = Sets.newHashSet();
+    for (ConstraintViolationSummary cvs : cvsList) {
+      violationDescs.add(cvs.getViolationDescription());
+    }
+    assertEquals(ImmutableSet.of("ViolationCode1", "ViolationCode2"), violationDescs);
+  }
+
+  @Test
+  public void testCheckException() {
+    expect(extent.contains(anyObject(BinaryComparable.class))).andReturn(true);
+    replayAll();
+    constraints.add(makeExceptionConstraint());
+    ConstraintViolationSummary cvs = Iterables.getOnlyElement(cc.check(env, m).asList());
+    assertEquals("CONSTRAINT FAILED : threw some Exception", cvs.getViolationDescription());
+  }
+}


Mime
View raw message