You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ho...@apache.org on 2016/03/21 01:44:11 UTC

[49/50] lucene-solr:jira/SOLR-445: SOLR-445: cleanup some simple nocommits

SOLR-445: cleanup some simple nocommits


Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/6ec8c635
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/6ec8c635
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/6ec8c635

Branch: refs/heads/jira/SOLR-445
Commit: 6ec8c635bf5853dfb229f89cb2818749c1cfe8ce
Parents: 1aa1ba3
Author: Chris Hostetter <ho...@apache.org>
Authored: Sun Mar 20 16:05:52 2016 -0700
Committer: Chris Hostetter <ho...@apache.org>
Committed: Sun Mar 20 16:05:52 2016 -0700

----------------------------------------------------------------------
 .../processor/DistributedUpdateProcessor.java   |  24 +--
 .../processor/TolerantUpdateProcessor.java      |  29 ++--
 .../TolerantUpdateProcessorFactory.java         |  20 ++-
 .../conf/solrconfig-tolerant-update-minimal.xml |  40 +++++
 .../org/apache/solr/core/TestBadConfig.java     |   5 +
 .../processor/TolerantUpdateProcessorTest.java  |  14 +-
 .../solr/common/ToleratedUpdateError.java       |  24 ++-
 .../solr/common/TestToleratedUpdateError.java   | 160 ++++++++++++++-----
 8 files changed, 248 insertions(+), 68 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
index 01aa38b..0c7836e 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
@@ -1722,19 +1722,23 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
     private static final int buildCode(List<Error> errors) {
       assert null != errors;
       assert 0 < errors.size();
-      
-      // if they are all the same, then we use that...
-      int result = errors.get(0).statusCode;
+
+      int minCode = Integer.MAX_VALUE;
+      int maxCode = Integer.MIN_VALUE;
       for (Error error : errors) {
         log.trace("REMOTE ERROR: {}", error);
-        if (result != error.statusCode ) {
-          // ...otherwise use sensible default
-          return ErrorCode.SERVER_ERROR.code;
-          // nocommit: don't short circut - check them all...
-          // nocommit: ...even if not all same, use 400 if all 4xx, else use 500
-        }
+        minCode = Math.min(error.statusCode, minCode);
+        maxCode = Math.max(error.statusCode, maxCode);
       }
-      return result;
+      if (minCode == maxCode) {
+        // all codes are consistent, use that...
+        return minCode;
+      } else if (400 <= minCode && maxCode < 500) {
+        // all codes are 4xx, use 400
+        return ErrorCode.BAD_REQUEST.code;
+      } 
+      // ...otherwise use sensible default
+      return ErrorCode.SERVER_ERROR.code;
     }
     
     /** Helper method for constructor */

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessor.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessor.java
index e53d33c..78457bb 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessor.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessor.java
@@ -77,10 +77,11 @@ import org.slf4j.LoggerFactory;
  */
 public class TolerantUpdateProcessor extends UpdateRequestProcessor {
   private static final Logger log = LoggerFactory.getLogger(TolerantUpdateProcessor.class);
+  
   /**
-   * String to be used as document key in the response if a real ID can't be determined
+   * String to be used as document key for errors when a real uniqueKey can't be determined
    */
-  private static final String UNKNOWN_ID = "(unknown)"; // nocommit: fail hard and fast if no uniqueKey
+  private static final String UNKNOWN_ID = "(unknown)"; 
 
   /**
    * Response Header
@@ -93,7 +94,10 @@ public class TolerantUpdateProcessor extends UpdateRequestProcessor {
    * batch
    */
   private final int maxErrors;
-  
+
+  /** The uniqueKey field */
+  private SchemaField uniqueKeyField;
+
   private final SolrQueryRequest req;
   private ZkController zkController;
 
@@ -137,8 +141,8 @@ public class TolerantUpdateProcessor extends UpdateRequestProcessor {
     assert ! DistribPhase.FROMLEADER.equals(distribPhase);
     
     this.zkController = this.req.getCore().getCoreDescriptor().getCoreContainer().getZkController();
-
-    // nocommit: assert existence of uniqueKey field & record for future use
+    this.uniqueKeyField = this.req.getCore().getLatestSchema().getUniqueKeyField();
+    assert null != uniqueKeyField : "Factory didn't enforce uniqueKey field?";
   }
   
   @Override
@@ -160,7 +164,7 @@ public class TolerantUpdateProcessor extends UpdateRequestProcessor {
         
         knownErrors.add(new ToleratedUpdateError
                         (CmdType.ADD,
-                         getPrintableId(id, cmd.getReq().getSchema().getUniqueKeyField()),
+                         getPrintableId(id),
                          t.getMessage()));
         if (knownErrors.size() > maxErrors) {
           firstErrTracker.throwFirst();
@@ -319,15 +323,14 @@ public class TolerantUpdateProcessor extends UpdateRequestProcessor {
    * Returns the output of {@link org.apache.solr.schema.FieldType#
    * indexedToReadable(BytesRef, CharsRefBuilder)} of the field
    * type of the uniqueKey on the {@link BytesRef} passed as parameter.
-   * <code>ref</code> should be the indexed representation of the id and
-   * <code>field</code> should be the uniqueKey schema field. If any of
-   * the two parameters is null this method will return {@link #UNKNOWN_ID}
+   * <code>ref</code> should be the indexed representation of the id -- if null
+   * (possibly because it's missing in the update) this method will return {@link #UNKNOWN_ID}
    */
-  private String getPrintableId(BytesRef ref, SchemaField field) {
-    if(ref == null || field == null) {
-      return UNKNOWN_ID; // nocommit: fail hard and fast
+  private String getPrintableId(BytesRef ref) {
+    if (ref == null) {
+      return UNKNOWN_ID;
     }
-    return field.getType().indexedToReadable(ref, new CharsRefBuilder()).toString();
+    return uniqueKeyField.getType().indexedToReadable(ref, new CharsRefBuilder()).toString();
   }
 
   // nocommit: 1) is this method even needed? 2) is this method correct? 3) javadocs

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessorFactory.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessorFactory.java b/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessorFactory.java
index d049077..35ca63b 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessorFactory.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/TolerantUpdateProcessorFactory.java
@@ -19,9 +19,12 @@ package org.apache.solr.update.processor;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.SolrCore;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.schema.SchemaField;
 import org.apache.solr.update.processor.DistributedUpdateProcessor.DistribPhase;
+import org.apache.solr.util.plugin.SolrCoreAware;
 
 import static org.apache.solr.update.processor.DistributingUpdateProcessorFactory.DISTRIB_UPDATE_PARAM;
 
@@ -68,9 +71,7 @@ import static org.apache.solr.update.processor.DistributingUpdateProcessorFactor
  * 
  */
 public class TolerantUpdateProcessorFactory extends UpdateRequestProcessorFactory
-    implements UpdateRequestProcessorFactory.RunAlways {
-
-  // nocommit: make SolrCoreAware and fail fast if no uniqueKey configured
+  implements SolrCoreAware, UpdateRequestProcessorFactory.RunAlways {
   
   /**
    * Parameter that defines how many errors the UpdateRequestProcessor will tolerate
@@ -82,6 +83,8 @@ public class TolerantUpdateProcessorFactory extends UpdateRequestProcessorFactor
    * or in the request
    */
   private int defaultMaxErrors = Integer.MAX_VALUE;
+
+  private boolean informed = false;
   
   @SuppressWarnings("rawtypes")
   @Override
@@ -101,8 +104,19 @@ public class TolerantUpdateProcessorFactory extends UpdateRequestProcessorFactor
   }
   
   @Override
+  public void inform(SolrCore core) {
+    informed = true;
+    if (null == core.getLatestSchema().getUniqueKeyField()) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, this.getClass().getName() +
+                              " requires a schema that includes a uniqueKey field.");
+    }
+  }
+
+  @Override
   public UpdateRequestProcessor getInstance(SolrQueryRequest req, SolrQueryResponse rsp, UpdateRequestProcessor next) {
 
+    assert informed : "inform(SolrCore) never called?";
+    
     // short circut if we're a replica processing commands from our leader
     DistribPhase distribPhase = DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM));
     if (DistribPhase.FROMLEADER.equals(distribPhase)) {

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-update-minimal.xml
----------------------------------------------------------------------
diff --git a/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-update-minimal.xml b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-update-minimal.xml
new file mode 100644
index 0000000..d3b90db
--- /dev/null
+++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-update-minimal.xml
@@ -0,0 +1,40 @@
+<?xml version="1.0" ?>
+
+<!--
+ 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.
+-->
+
+<config>
+
+  <luceneMatchVersion>${tests.luceneMatchVersion:LATEST}</luceneMatchVersion>
+  
+  <schemaFactory class="ClassicIndexSchemaFactory"/>
+  <xi:include href="solrconfig.snippet.randomindexconfig.xml" xmlns:xi="http://www.w3.org/2001/XInclude"/>
+  <requestHandler name="/select" class="solr.SearchHandler">
+    <lst name="defaults">
+      <str name="echoParams">explicit</str>
+      <str name="indent">true</str>
+      <str name="df">text</str>
+    </lst>
+  </requestHandler>
+  
+  <updateRequestProcessorChain name="tolerant-chain">
+    <processor class="solr.TolerantUpdateProcessorFactory" />
+    <processor class="solr.RunUpdateProcessorFactory" />
+  </updateRequestProcessorChain>
+
+</config>
+

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/test/org/apache/solr/core/TestBadConfig.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/core/TestBadConfig.java b/solr/core/src/test/org/apache/solr/core/TestBadConfig.java
index 637c975..31361be 100644
--- a/solr/core/src/test/org/apache/solr/core/TestBadConfig.java
+++ b/solr/core/src/test/org/apache/solr/core/TestBadConfig.java
@@ -96,4 +96,9 @@ public class TestBadConfig extends AbstractBadConfigTestBase {
     assertConfigs("bad-solrconfig-unexpected-schema-attribute.xml", "schema-minimal.xml",
                   "Unexpected arg(s): {bogusParam=bogusValue}");
   }
+
+  public void testTolerantUpdateProcessorNoUniqueKey() throws Exception {
+    assertConfigs("solrconfig-tolerant-update-minimal.xml", "schema-minimal.xml",
+                  "requires a schema that includes a uniqueKey field");
+  }
 }

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java b/solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java
index a519068..9bbead8 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java
@@ -242,7 +242,19 @@ public class TolerantUpdateProcessorTest extends UpdateProcessorTestBase {
         ,"//result[@numFound='6']");
   }
 
-  // nocommit: need a testMaxErrorsNegative (ie: infinite)
+  @Test
+  public void testMaxErrorsInfinite() throws IOException {
+    ModifiableSolrParams requestParams = new ModifiableSolrParams();
+    requestParams.add("maxErrors", "-1");
+    try {
+      assertAddsSucceedWithErrors("tolerant-chain-max-errors-not-set", docs, null, badIds);
+    } catch(Exception e) {
+      fail("Shouldn't get an exception for this batch: " + e.getMessage());
+    }
+    assertU(commit());
+    assertQ(req("q","*:*")
+            ,"//result[@numFound='10']");
+  }
   
   @Test
   public void testMaxErrors0() throws IOException {

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/solrj/src/java/org/apache/solr/common/ToleratedUpdateError.java
----------------------------------------------------------------------
diff --git a/solr/solrj/src/java/org/apache/solr/common/ToleratedUpdateError.java b/solr/solrj/src/java/org/apache/solr/common/ToleratedUpdateError.java
index c6c4244..6da6fc5 100644
--- a/solr/solrj/src/java/org/apache/solr/common/ToleratedUpdateError.java
+++ b/solr/solrj/src/java/org/apache/solr/common/ToleratedUpdateError.java
@@ -19,10 +19,13 @@ package org.apache.solr.common;
 import java.util.ArrayList;
 import java.util.List;
 import org.apache.solr.common.util.SimpleOrderedMap;
-
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 
 /**
- * nocommit: more javadocs, mention (but obviously no link) to TolerantUpdateProcessor
+ * Models the basic information related to a single "tolerated" error that occured during updates.  
+ * This class is only useful when the <code>ToleranteUpdateProcessorFactory</code> is used in an update 
+ * processor chain
  */
 public final class ToleratedUpdateError {
     
@@ -71,8 +74,17 @@ public final class ToleratedUpdateError {
    * @see #getSimpleMap
    */
   public static ToleratedUpdateError parseMap(SimpleOrderedMap<String> data) {
-    // nocommit: error handling and clean exception reporting if data is bogus
-    return new ToleratedUpdateError(CmdType.valueOf(data.get("type")), data.get("id"), data.get("message"));
+    final String id = data.get("id");
+    final String message = data.get("message");
+    final String t = data.get("type");
+    if (null == t || null == id || null == message) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, "Map does not represent a ToleratedUpdateError, must contain 'type', 'id', and 'message'");
+    }
+    try {
+      return new ToleratedUpdateError(CmdType.valueOf(t), id, message);
+    } catch (IllegalArgumentException iae) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, "Invalid type for ToleratedUpdateError: " + t, iae);
+    }
   }
   
   /** 
@@ -86,7 +98,9 @@ public final class ToleratedUpdateError {
       return null; // not a key we care about
     }
     final int typeEnd = metadataKey.indexOf(':', META_PRE_LEN);
-    assert 0 < typeEnd; // nocommit: better error handling
+    if (typeEnd < 0) {
+      return null; // has our prefix, but not our format -- must not be a key we (actually) care about
+    }
     return new ToleratedUpdateError(CmdType.valueOf(metadataKey.substring(META_PRE_LEN, typeEnd)),
                                     metadataKey.substring(typeEnd+1), metadataVal);
   }

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/6ec8c635/solr/solrj/src/test/org/apache/solr/common/TestToleratedUpdateError.java
----------------------------------------------------------------------
diff --git a/solr/solrj/src/test/org/apache/solr/common/TestToleratedUpdateError.java b/solr/solrj/src/test/org/apache/solr/common/TestToleratedUpdateError.java
index 91636b3..a76443a 100644
--- a/solr/solrj/src/test/org/apache/solr/common/TestToleratedUpdateError.java
+++ b/solr/solrj/src/test/org/apache/solr/common/TestToleratedUpdateError.java
@@ -16,67 +16,120 @@
  */
 package org.apache.solr.common;
 
+import java.util.EnumSet;
 import org.apache.solr.common.ToleratedUpdateError;
 import org.apache.solr.common.ToleratedUpdateError.CmdType;
+import org.apache.solr.common.util.SimpleOrderedMap;
 
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.TestUtil;
 
 /** Basic testing of the serialization/encapsulation code in ToleratedUpdateError */
 public class TestToleratedUpdateError extends LuceneTestCase {
   
-  // nocommit: add randomized testing, particularly with non-trivial 'id' values
+  private final static CmdType[] ALL_TYPES = EnumSet.allOf(CmdType.class).toArray(new CmdType[0]);
+  
+  public void testBasics() {
+    
+    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
+                (new ToleratedUpdateError(CmdType.ADD, "doc2", "some error")));
+    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
+                (new ToleratedUpdateError(CmdType.ADD, "doc1", "some errorxx")));
+    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
+                (new ToleratedUpdateError(CmdType.DELID, "doc1", "some error")));
+  }
 
-  public void checkRoundTripComparisons(Coppier coppier) {
+  public void testParseMetadataErrorHandling() {
 
     assertNull(ToleratedUpdateError.parseMetadataIfToleratedUpdateError("some other key", "some value"));
+
+    // see if someone tries to trick us into having an NPE...
+    ToleratedUpdateError valid = new ToleratedUpdateError(CmdType.ADD, "doc2", "some error");
+    String badKey = valid.getMetadataKey().replace(":", "X");
+    assertNull(ToleratedUpdateError.parseMetadataIfToleratedUpdateError(badKey, valid.getMetadataValue()));
+  }
+  
+  public void testParseMapErrorChecking() {
+    SimpleOrderedMap<String> bogus = new SimpleOrderedMap<String>();
+    try {
+      ToleratedUpdateError.parseMap(bogus);
+      fail("map should not be parsable");
+    } catch (SolrException e) {
+      assertTrue(e.toString(), e.getMessage().contains("Map does not represent a ToleratedUpdateError") );
+    }
+
+    bogus.add("id", "some id");
+    bogus.add("message", "some message");
+    try {
+      ToleratedUpdateError.parseMap(bogus);
+      fail("map should still not be parsable");
+    } catch (SolrException e) {
+      assertTrue(e.toString(), e.getMessage().contains("Map does not represent a ToleratedUpdateError") );
+    }
     
+    bogus.add("type", "not a real type");
+    try {
+      ToleratedUpdateError.parseMap(bogus);
+      fail("invalid type should not be parsable");
+    } catch (SolrException e) {
+      assertTrue(e.toString(), e.getMessage().contains("Invalid type")); 
+    }
+  }
+  
+  public void testParseMap() {
+    // trivial
+    SimpleOrderedMap valid = new SimpleOrderedMap<String>();
+    valid.add("type", CmdType.ADD.toString());
+    valid.add("id", "some id");
+    valid.add("message", "some message");
+    
+    ToleratedUpdateError in = ToleratedUpdateError.parseMap(valid);
+    compare(in, MAP_COPPIER);
+    compare(in, METADATA_COPPIER);
+
+    // randomized
+    int numIters = atLeast(5000);
+    for (int i = 0; i < numIters; i++) {
+      valid = new SimpleOrderedMap<String>();
+      valid.add("type", ALL_TYPES[TestUtil.nextInt(random(), 0, ALL_TYPES.length-1)].toString());
+      valid.add("id", TestUtil.randomUnicodeString(random()));
+      valid.add("message", TestUtil.randomUnicodeString(random()));
+      
+      in = ToleratedUpdateError.parseMap(valid);
+      compare(in, MAP_COPPIER);
+      compare(in, METADATA_COPPIER);
+    }
+  }
+  
+  public void checkRoundTripComparisons(Coppier coppier) {
+
+    // some simple basics
     for (ToleratedUpdateError in : new ToleratedUpdateError[] {
         new ToleratedUpdateError(CmdType.ADD, "doc1", "some error"),
         new ToleratedUpdateError(CmdType.DELID, "doc1", "some diff error"),
         new ToleratedUpdateError(CmdType.DELQ, "-field:yakko other_field:wakko", "some other error"),
       }) {
       
-      ToleratedUpdateError out = coppier.copy(in);
-      
-      assertNotNull(out);
-      assertEquals(out.type, in.type);
-      assertEquals(out.id, in.id);
-      assertEquals(out.errorValue, in.errorValue);
-      assertEquals(out.hashCode(), in.hashCode());
-      assertEquals(out.toString(), in.toString());
-
-      assertEquals(in.getMetadataKey(), out.getMetadataKey());
-      assertEquals(in.getMetadataValue(), out.getMetadataValue());
-      
-      assertEquals(out, in);
-      assertEquals(in, out);
+      compare(in, coppier);
+    }
 
+    // randomized testing of non trivial keys/values
+    int numIters = atLeast(5000);
+    for (int i = 0; i < numIters; i++) {
+      ToleratedUpdateError in = new ToleratedUpdateError
+        (ALL_TYPES[TestUtil.nextInt(random(), 0, ALL_TYPES.length-1)],
+         TestUtil.randomUnicodeString(random()),
+         TestUtil.randomUnicodeString(random()));
+      compare(in, coppier);
     }
-    
-    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
-                (new ToleratedUpdateError(CmdType.ADD, "doc2", "some error")));
-    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
-                (new ToleratedUpdateError(CmdType.ADD, "doc1", "some errorxx")));
-    assertFalse((new ToleratedUpdateError(CmdType.ADD, "doc1", "some error")).equals
-                (new ToleratedUpdateError(CmdType.DELID, "doc1", "some error")));
-    
   }
-  
+
   public void testMetadataRoundTripComparisons(Coppier coppier) {
-    checkRoundTripComparisons(new Coppier() {
-      public ToleratedUpdateError copy(ToleratedUpdateError in) {
-        return ToleratedUpdateError.parseMetadataIfToleratedUpdateError
-          (in.getMetadataKey(), in.getMetadataValue());
-      }
-    });
+    checkRoundTripComparisons(METADATA_COPPIER);
   }
   
   public void testMapRoundTripComparisons() {
-    checkRoundTripComparisons(new Coppier() {
-      public ToleratedUpdateError copy(ToleratedUpdateError in) {
-        return ToleratedUpdateError.parseMap(in.getSimpleMap());
-      }
-    });
+    checkRoundTripComparisons(MAP_COPPIER);
   }
 
   /** trivial sanity check */
@@ -95,9 +148,44 @@ public class TestToleratedUpdateError extends LuceneTestCase {
     
   }
 
+  public void compare(ToleratedUpdateError in, Coppier coppier) {
+      ToleratedUpdateError out = coppier.copy(in);
+      assertNotNull(out);
+      compare(in, out);
+  }
+  
+  public void compare(ToleratedUpdateError in, ToleratedUpdateError out) {
+    assertEquals(out.type, in.type);
+    assertEquals(out.id, in.id);
+    assertEquals(out.errorValue, in.errorValue);
+    
+    assertEquals(out.hashCode(), in.hashCode());
+    assertEquals(out.toString(), in.toString());
+    
+    assertEquals(in.getMetadataKey(), out.getMetadataKey());
+    assertEquals(in.getMetadataValue(), out.getMetadataValue());
+    
+    assertEquals(out, in);
+    assertEquals(in, out);
+  }
+  
   private static abstract class Coppier {
     public abstract ToleratedUpdateError copy(ToleratedUpdateError in);
   }
+
+  private static final Coppier MAP_COPPIER = new Coppier() {
+    public ToleratedUpdateError copy(ToleratedUpdateError in) {
+      return ToleratedUpdateError.parseMap(in.getSimpleMap());
+    }
+  };
+  
+  private static final Coppier METADATA_COPPIER = new Coppier() {
+    public ToleratedUpdateError copy(ToleratedUpdateError in) {
+      return ToleratedUpdateError.parseMetadataIfToleratedUpdateError
+        (in.getMetadataKey(), in.getMetadataValue());
+    }
+  };
+  
 }