Introduce incremental push logic into AdAdaptor
diff --git a/src/com/google/enterprise/adaptor/ad/AdAdaptor.java b/src/com/google/enterprise/adaptor/ad/AdAdaptor.java
index 3ea71be..418f591 100644
--- a/src/com/google/enterprise/adaptor/ad/AdAdaptor.java
+++ b/src/com/google/enterprise/adaptor/ad/AdAdaptor.java
@@ -26,13 +26,24 @@
 import com.google.enterprise.adaptor.Response;
 import com.google.enterprise.adaptor.UserPrincipal;
 
-import java.io.*;
+import java.io.IOException;
 import java.text.MessageFormat;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import javax.naming.InterruptedNamingException;
+import javax.naming.NamingException;
 
 /** Adaptor for Active Directory. */
 public class AdAdaptor extends AbstractAdaptor
@@ -40,6 +51,13 @@
   private static final Logger log
       = Logger.getLogger(AdAdaptor.class.getName());
   private static final boolean CASE_SENSITIVITY = false;
+  /**
+   * Only one crawl (full or incremental) is done at a time, however:
+   * when a full crawl is invoked, we wait until the lock is available;
+   * when an incremental crawl is invoked, we immediately return if the lock
+   * isn't available.
+   */
+  private final ReentrantLock mutex = new ReentrantLock();
 
   private String namespace;
   private String defaultUser;  // used if an AD doesn't override
@@ -47,6 +65,7 @@
   private List<AdServer> servers = new ArrayList<AdServer>();
   private Map<String, String> localizedStrings;
   private boolean feedBuiltinGroups;
+  private GroupCatalog lastCompleteGroupCatalog = null;
 
   @Override
   public void initConfig(Config config) {
@@ -116,9 +135,9 @@
   }
 
   /**
-    * This method exists specifically to be overwritten in the test class, in
-    * order to inject a version of AdServer that uses mocks.
-    */
+   * This method exists specifically to be overwritten in the test class, in
+   * order to inject a version of AdServer that uses mocks.
+   */
   @VisibleForTesting
   AdServer newAdServer(Method method, String host, int port,
       String principal, String passwd) {
@@ -136,51 +155,105 @@
     AbstractAdaptor.main(new AdAdaptor(), args);
   }
 
-  /** Pushes all groups from all AdServers. */
+  /** Crawls/pushes all groups from all AdServers. */
+
   @Override
   public void getDocIds(DocIdPusher pusher) throws InterruptedException,
       IOException {
-    readFromAllServers(pusher, false);
+    log.log(Level.FINER, "getDocIds invoked - waiting for lock.");
+    mutex.lock();
+    try {
+      clearLastCompleteGroupCatalog();
+      GroupCatalog cumulativeCatalog = makeFullCatalog();
+      // all servers were able to successfully populate the catalog: do a push
+      cumulativeCatalog.resolveForeignSecurityPrincipals();
+      Map<GroupPrincipal, List<Principal>> groups =
+          cumulativeCatalog.makeDefs();
+      pusher.pushGroupDefinitions(groups, CASE_SENSITIVITY);
+      // TODO(myk): clear membership information from cache - retain only the
+      // entities in bySid, byDn, and wellKnownMembership.
+      lastCompleteGroupCatalog = cumulativeCatalog;
+    } finally {
+      mutex.unlock();
+      log.log(Level.FINE, "getDocIds ending - lock released.");
+    }
+  }
+
+  private GroupCatalog makeFullCatalog() throws InterruptedException,
+      IOException {
+    GroupCatalog cumulativeCatalog = new GroupCatalog(localizedStrings,
+        namespace, feedBuiltinGroups);
+    for (AdServer server : servers) {
+      try {
+        server.ensureConnectionIsCurrent();
+        GroupCatalog catalog = new GroupCatalog(localizedStrings, namespace,
+              feedBuiltinGroups);
+        catalog.readEverythingFrom(server);
+        cumulativeCatalog.add(catalog);
+      } catch (NamingException ne) {
+        String host = server.getHostName();
+        throw new IOException("could not get entities from " + host, ne);
+      }
+    }
+    return cumulativeCatalog;
   }
 
   /**
-   * Attempts an incremntal push of all groups from all AdServers
-   *
-   * When a server cannot do an incremental push, it does a full push.
+   * Attempts an incremental push of updated groups from all AdServers.
+   * <p>
+   * When a server cannot do an incremental push, it does a full crawl without
+   * doing a push afterwards -- this sets up its state so that subsequent
+   * incremental pushes can work.  A future version of this method will do the
+   * full crawl ignoring the "member" attribute.
    */
   @Override
   public void getModifiedDocIds(DocIdPusher pusher) throws InterruptedException,
       IOException {
-    readFromAllServers(pusher, true);
+    if (!mutex.tryLock()) {
+      log.log(Level.FINE, "getModifiedDocIds could not acquire lock; " +
+          "will retry later.");
+      return;
+    }
+    try {
+      log.log(Level.FINE, "getModifiedDocIds starting - acquired lock.");
+
+      for (AdServer server : servers) {
+        String previousServiceName = server.getDsServiceName();
+        String previousInvocationId = server.getInvocationID();
+        long previousHighestUSN = server.getHighestCommittedUSN();
+        try {
+          server.ensureConnectionIsCurrent();
+          // TODO(myk): combine each server's resulting Entities into a single
+          lastCompleteGroupCatalog.readUpdatesFrom(server, previousServiceName,
+              previousInvocationId, previousHighestUSN);
+        } catch (NamingException ne) {
+          // invalidate the saved group catalog
+          clearLastCompleteGroupCatalog();
+          String host = server.getHostName();
+          throw new IOException("could not get entities from " + host, ne);
+        }
+      }
+
+      // all servers were able to successfully update the catalog: do a push
+      lastCompleteGroupCatalog.resolveForeignSecurityPrincipals();
+      Map<GroupPrincipal, List<Principal>> groups =
+          lastCompleteGroupCatalog.makeDefs();
+      // TODO(myk): pass on (only) new/modified entities to resolve/makeDefs,
+      // so that we're only pushing the new/modified groups below.
+      pusher.pushGroupDefinitions(groups, CASE_SENSITIVITY);
+      // TODO(myk): clear membership information from cache - retain only the
+      // entities in bySid, byDn, and wellKnownMembership.
+    } finally {
+      mutex.unlock();
+      log.log(Level.FINE, "getModifiedDocIds ending - lock released.");
+    }
   }
 
-  public void readFromAllServers(DocIdPusher pusher,
-      boolean doIncrementalPushIfPossible)
-      throws InterruptedException, IOException {
-    // TODO(pjo): implement built in groups
-    GroupCatalog cumulativeCatalog = new GroupCatalog(localizedStrings,
-        namespace, feedBuiltinGroups);
-    for (AdServer server : servers) {
-      String previousServiceName = server.getDsServiceName();
-      String previousInvocationId = server.getInvocationID();
-      long previousHighestUSN = server.getHighestCommittedUSN();
-      server.initialize();
-      try {
-        GroupCatalog catalog = new GroupCatalog(localizedStrings, namespace,
-            feedBuiltinGroups);
-        catalog.readFrom(server, doIncrementalPushIfPossible,
-            previousServiceName, previousInvocationId, previousHighestUSN);
-        cumulativeCatalog.add(catalog);
-      } catch (InterruptedNamingException ine) {
-        String host = server.getHostName();
-        throw new IOException("could not get entities from " + host, ine);
-      }
-    }
-    cumulativeCatalog.resolveForeignSecurityPrincipals();
-    Map<GroupPrincipal, List<Principal>> groups = cumulativeCatalog.makeDefs();
-    cumulativeCatalog.clear();
-    cumulativeCatalog = null;
-    pusher.pushGroupDefinitions(groups, CASE_SENSITIVITY);
+  // don't expose the <code>lastCompleteGroupCatalog</code> field, but do allow
+  // tests to clear it
+  @VisibleForTesting
+  void clearLastCompleteGroupCatalog() {
+    lastCompleteGroupCatalog = null;
   }
 
   // Space for all group info, organized in different ways
@@ -262,47 +335,11 @@
       this.domain.putAll(domain);
     }
 
-    void readFrom(AdServer server, boolean doIncrementalPushIfPossible,
-        String previousServiceName, String previousInvocationId,
-        long previousHighestUSN) throws InterruptedNamingException {
-      if (!doIncrementalPushIfPossible) {
-        log.log(Level.INFO, "Starting full crawl.");
-        fullCrawl(server);
-        return;
-      }
-      // TODO(myk): Determine whether adaptors should include code to get/set
-      // last full sync time, and if exceeding some threshhold should force a
-      // full crawl.
-      String currentServiceName = server.getDsServiceName();
-      String currentInvocationId = server.getInvocationID();
-      long currentHighestUSN = server.getHighestCommittedUSN();
-      if (!currentServiceName.equals(previousServiceName)) {
-        log.log(Level.WARNING,
-            "Directory Controller changed from {0} to {1} -- performing full "
-            + "recrawl.  Consider configuring AD server to connect directly to "
-            + "FQDN address of domain controller for partial updates support.",
-            new Object[]{previousServiceName, currentServiceName});
-        fullCrawl(server);
-        return;
-      }
-      if (!currentInvocationId.equals(previousInvocationId)) {
-        log.log(Level.WARNING,
-            "Directory Controller {0} has been restored from backup.  "
-            + "Performing full recrawl.", currentServiceName);
-        fullCrawl(server);
-        return;
-      }
-      if (currentHighestUSN == previousHighestUSN) {
-        log.log(Level.INFO, "No updates on server {0} -- no crawl invoked.",
-            server);
-        return;
-      }
-      log.log(Level.INFO, "Starting incremental crawl.");
-      incrementalCrawl(server, previousHighestUSN, currentHighestUSN);
-    }
-
     @VisibleForTesting
-    void fullCrawl(AdServer server) throws InterruptedNamingException {
+    void readEverythingFrom(AdServer server) throws InterruptedNamingException {
+      // TODO(myk): Phase II: indicate whether or not this search should
+      // include members
+      log.log(Level.FINE, "Starting full crawl.");
       // LDAP_MATCHING_RULE_BIT_AND = 1.2.840.113556.1.4.803
       // and ADS_GROUP_TYPE_SECURITY_ENABLED = 2147483648.
       entities = server.search("(|(&(objectClass=group)"
@@ -313,25 +350,97 @@
               "objectSid;binary", "userPrincipalName", "primaryGroupId",
               "member", "userAccountControl" });
       // disabled groups handled later, in makeDefs()
+      log.log(Level.FINE, "Ending full crawl - now starting processing.");
+      processEntities(entities, server.getnETBIOSName());
+    }
+
+    /**
+     * Do an AD search for only groups/users that have been updated since the
+     * previous full or incremental search.
+     * <p>If either <code>getDsServiceName()</code> or
+     * <code>server.getInvocationID()</code> have changed, the cache is stale
+     * and (only) a full crawl is done, to refresh the cache.  If neither have
+     * changed, then only groups/users that have a <code>uSNChanged</code>
+     * attribute newer than the <code>previousHighestUSN</code> parameter are
+     * retrieved and returned.
+     * @param server the Active Directory server to query
+     * @param previousServiceName last-crawled value of
+     *     <code>getDsServiceName()</code>
+     * @param previousInvocationId last-crawled value of
+     *     <code>server.getInvocationID()</code>
+     * @param previousHighestUSN last-crawled value of
+     *     <code>server.getHighestCommittedUSN()</code>
+     * <code>previousHighestUSN</code>.
+     */
+     /* TODO(myk): add
+     * @return all instances of <code>AdEntity</code> that are users/groups that
+     *     have a <code>uSNChanged</code> attribute newer than, or
+     *     <code>Collections.emptySet()</code> when the cache had been stale.
+     */
+    @VisibleForTesting
+    void readUpdatesFrom(AdServer server, String previousServiceName,
+        String previousInvocationId, long previousHighestUSN)
+        throws InterruptedNamingException {
+      // TODO(myk): Determine whether adaptors should include code to get/set
+      // last full sync time, and if exceeding some threshhold should force a
+      // full crawl.
+      String currentServiceName = server.getDsServiceName();
+      String currentInvocationId = server.getInvocationID();
+      long currentHighestUSN = server.getHighestCommittedUSN();
+      if (!currentServiceName.equals(previousServiceName)) {
+        // only log a warning if previous service name was set to something
+        if (previousServiceName != null) {
+          log.log(Level.WARNING, "Directory Controller changed from {0} to {1} "
+              + "-- performing full recrawl.  Consider configuring AD server to"
+              + " connect directly to FQDN address of domain controller for "
+              + "partial updates support.",
+              new Object[]{previousServiceName, currentServiceName});
+        }
+        readEverythingFrom(server);
+        return;
+        //TODO(myk): return Collections.emptySet();
+      }
+      if (!currentInvocationId.equals(previousInvocationId)) {
+        log.log(Level.WARNING,
+            "Directory Controller {0} has been restored from backup.  "
+            + "Performing full recrawl.", currentServiceName);
+        readEverythingFrom(server);
+        return;
+        //TODO(myk): return Collections.emptySet();
+      }
+      if (currentHighestUSN == previousHighestUSN) {
+        log.log(Level.INFO, "No updates on server {0} -- no crawl invoked.",
+            server);
+        return;
+        //TODO(myk): return Collections.emptySet();
+      }
+      log.log(Level.INFO, "Attempting incremental crawl.");
+      incrementalCrawl(server, previousHighestUSN, currentHighestUSN);
+    }
+
+    private void processEntities(Set<AdEntity> entities, String nETBIOSName) {
       log.log(Level.FINE, "received {0} entities from server", entities.size());
       for (AdEntity e : entities) {
         bySid.put(e.getSid(), e);
         byDn.put(e.getDn(), e);
         // TODO(pjo): Have AdServer put domain into AdEntity during search
         domain.put(e, e.getSid().startsWith("S-1-5-32-") ?
-            localizedStrings.get("Builtin") : server.getnETBIOSName());
+            localizedStrings.get("Builtin") : nETBIOSName);
       }
-      initializeMembers();
-      resolvePrimaryGroups();
+      initializeMembers(entities);
+      resolvePrimaryGroups(entities);
+      log.log(Level.FINE, "Ending processing of {0} entities", entities.size());
     }
 
     @VisibleForTesting
-    void incrementalCrawl(AdServer server, long previousHighestUSN,
+    Set<AdEntity> incrementalCrawl(AdServer server, long previousHighestUSN,
         long currentHighestUSN) throws InterruptedNamingException {
+      log.log(Level.FINE, "Starting incremental crawl.");
       // LDAP_MATCHING_RULE_BIT_AND = 1.2.840.113556.1.4.803
       // and ADS_GROUP_TYPE_SECURITY_ENABLED = 2147483648.
-      Set<AdEntity> newEntities = server.search("(&(uSNChanged>"
-          + previousHighestUSN + ")(|(&(objectClass=group)"
+      // TODO(myk): Phase II: indicate that this search should exclude members
+      Set<AdEntity> newOrModifiedEntities = server.search("(&(uSNChanged>="
+          + (previousHighestUSN + 1) + ")(|(&(objectClass=group)"
           + "(groupType:1.2.840.113556.1.4.803:=2147483648))"
           + "(&(objectClass=user)(objectCategory=person))))",
           /*deleted=*/ false,
@@ -339,24 +448,29 @@
               "objectSid;binary", "userPrincipalName", "primaryGroupId",
               "member", "userAccountControl" });
       // disabled groups handled later, in makeDefs()
-      log.log(Level.FINE, "received new {0} entities from server",
-          newEntities.size());
-      for (AdEntity e : newEntities) {
-        bySid.put(e.getSid(), e);
-        byDn.put(e.getDn(), e);
-        //TODO(myk): Handle tombstones / deleted items?
-        entities.add(e);
-        // TODO(pjo): Have AdServer put domain into AdEntity during search
-        domain.put(e, e.getSid().startsWith("S-1-5-32-") ?
-            localizedStrings.get("Builtin") : server.getnETBIOSName());
+      log.log(Level.FINE, "Ending incremental crawl - now starting "
+          + "processing.");
+      // remove previous value of newly-seen entity, if found
+      for (AdEntity e : newOrModifiedEntities) {
+        AdEntity oldEntity = bySid.get(e.getSid());
+        if (oldEntity != null) {
+          entities.remove(oldEntity);
+          byDn.remove(oldEntity.getDn());
+          members.remove(oldEntity);
+          wellKnownMembership.get(everyone).remove(oldEntity.getDn());
+        }
       }
-      // TODO(myk): determine if these routines should be optimized to handle
-      // only the newly-discovered entities.
-      initializeMembers();
-      resolvePrimaryGroups();
+      // add the new-or-modified entries to our catalog
+      entities.addAll(newOrModifiedEntities);
+      processEntities(newOrModifiedEntities, server.getnETBIOSName());
+      log.log(Level.FINE, "Ending incremental crawl.");
+      return newOrModifiedEntities;
     }
 
-    private void initializeMembers() {
+    /**
+     * Correctly specify each group's members in the "members" data store
+     */
+    private void initializeMembers(Set<AdEntity> entities) {
       for (AdEntity entity : entities) {
         if (entity.isGroup()) {
           members.put(entity, new TreeSet<String>(entity.getMembers()));
@@ -364,9 +478,14 @@
       }
     }
 
-    private void resolvePrimaryGroups() {
+    /**
+     * Make sure that each non-group entity's "primary" group exists in bySid
+     *
+     * and contains that entity in the <code>members</code> data store.
+     */
+    private void resolvePrimaryGroups(Set<AdEntity> entities) {
       int nadds = 0;
-      int missingGroups = 0;
+      int missing_groups = 0;  // TODO(myk): rename this so that lint is happy
       for (AdEntity e : entities) {
         if (e.isGroup()) {
           continue;
@@ -374,7 +493,7 @@
         AdEntity user = e;
         AdEntity primaryGroup = bySid.get(user.getPrimaryGroupSid());
         if (primaryGroup == null) {
-          missingGroups++;
+          missing_groups++;
           log.log(Level.WARNING,
               "Group {0} -- primary group for user {1} -- not found",
               new Object[]{user.getPrimaryGroupSid(), user});
@@ -388,13 +507,15 @@
         nadds++;
       }
       log.log(Level.FINE, "# primary groups: {0}", members.keySet().size());
-      if (missingGroups > 0) {
-        log.log(Level.FINE, "# missing primary groups: {0}", missingGroups);
+      if (missing_groups > 0) {
+        log.log(Level.FINE, "# missing primary groups: {0}", missing_groups);
       }
       log.log(Level.FINE, "# users added to all primary groups: {0}", nadds);
     }
 
     void resolveForeignSecurityPrincipals() {
+      //TODO(myk) Phase II: pass in only the entities just read in (for an
+      // incremental search to only consider those entities)
       int nGroups = 0;
       int nNullSid = 0;
       int nNullResolution = 0;
@@ -432,6 +553,8 @@
 
     Map<GroupPrincipal, List<Principal>> makeDefs() {
       // Merge members with well known group members
+      //TODO(myk) Phase II: pass in only the entities just read in (for an
+      // incremental search to only consider those entities)
       Map<AdEntity, Set<String>> allMembers
           = new HashMap<AdEntity, Set<String>>(members);
       allMembers.putAll(wellKnownMembership);
diff --git a/src/com/google/enterprise/adaptor/ad/AdServer.java b/src/com/google/enterprise/adaptor/ad/AdServer.java
index 0f353dd..4c9480c 100644
--- a/src/com/google/enterprise/adaptor/ad/AdServer.java
+++ b/src/com/google/enterprise/adaptor/ad/AdServer.java
@@ -17,17 +17,12 @@
 import com.google.common.annotations.VisibleForTesting;
 
 import java.io.IOException;
-import java.sql.Timestamp;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Hashtable;
-import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
-import javax.naming.AuthenticationException;
-import javax.naming.AuthenticationNotSupportedException;
 import javax.naming.CommunicationException;
 import javax.naming.Context;
 import javax.naming.InterruptedNamingException;
@@ -111,7 +106,7 @@
         "com.sun.jndi.ldap.LdapCtxFactory");
     // Connecting to configuration naming context is very slow for crawl users
     // in large multidomain environment, which belong to thousands of groups
-    // TODO: make this configurable
+    // TODO(pjo or myk): make this configurable
     env.put("com.sun.jndi.ldap.read.timeout", "90000");
     env.put(Context.SECURITY_AUTHENTICATION, "simple");
     env.put(Context.SECURITY_PRINCIPAL, principal);
@@ -136,11 +131,13 @@
   /**
    * Connects to the Active Directory server and retrieves AD configuration
    * information.
-   *
-   * This method is used for crawling as well as authorization of credentials
-   * against Active Directory.
+   * <p>This method is used for crawling as well as authorization of credentials
+   * against Active Directory.  Calling this method after a connection has been
+   * established will refresh the connection attributes (e.g.
+   * <code>highestCommittedUSN</code>).
    */
-  public void connect() throws CommunicationException, NamingException {
+  public void ensureConnectionIsCurrent()
+      throws CommunicationException, NamingException {
     Attributes attributes;
     try {
       attributes = ldapContext.getAttributes("");
@@ -160,7 +157,7 @@
 
   public void initialize() {
     try {
-      connect();
+      ensureConnectionIsCurrent();
       sid = AdEntity.getTextSid((byte[]) get(
           "distinguishedName=" + dn, "objectSid;binary", dn));
       invocationID = AdEntity.getTextGuid((byte[]) get(
@@ -193,7 +190,7 @@
   protected Object get(String filter, String attribute, String base) {
     searchCtls.setReturningAttributes(new String[] {attribute});
     try {
-      connect();  // re-establish LDAP connection, if necessary
+      ensureConnectionIsCurrent();
       NamingEnumeration<SearchResult> ldapResults =
           ldapContext.search(base, filter, searchCtls);
       if (!ldapResults.hasMore()) {
@@ -250,7 +247,7 @@
     searchCtls.setReturningAttributes(attributes);
     setControls(deleted);
     try {
-      connect();  // re-establish LDAP connection, if necessary
+      ensureConnectionIsCurrent();
       byte[] cookie = null;
       do {
         NamingEnumeration<SearchResult> ldapResults =
@@ -299,7 +296,7 @@
               "Retrieving additional groups for [" + g + "] " + memberRange);
           searchCtls.setReturningAttributes(new String[] {memberRange});
           NamingEnumeration<SearchResult> ldapResults = ldapContext.search(
-              dn, "(sAMAccountName=" + g.getSAMAccountName() +")", searchCtls);
+              dn, "(sAMAccountName=" + g.getSAMAccountName() + ")", searchCtls);
           SearchResult sr = ldapResults.next();
           int found = g.appendGroups(sr);
           start += found;
diff --git a/test/com/google/enterprise/adaptor/ad/AdAdaptorTest.java b/test/com/google/enterprise/adaptor/ad/AdAdaptorTest.java
index 83e1c54..108a187 100644
--- a/test/com/google/enterprise/adaptor/ad/AdAdaptorTest.java
+++ b/test/com/google/enterprise/adaptor/ad/AdAdaptorTest.java
@@ -14,7 +14,11 @@
 
 package com.google.enterprise.adaptor.ad;
 
-import static org.junit.Assert.*;
+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 com.google.common.collect.Sets;
 
@@ -30,15 +34,23 @@
 
 import org.junit.Test;
 
-import java.io.*;
+import java.io.IOException;
+import java.io.OutputStream;
 import java.net.URI;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
-import javax.naming.*;
-import javax.naming.directory.*;
-import javax.naming.ldap.*;
+import javax.naming.InterruptedNamingException;
 
 /** Test cases for {@link AdAdaptor}. */
 public class AdAdaptorTest {
@@ -133,8 +145,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     final AdEntity goldenEntity = new AdEntity("S-1-0-0",
         "cn=name\\ under,DN_for_default_naming_context");
@@ -188,8 +199,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     AdEntity[] groupEntity = groupCatalog.entities.toArray(new AdEntity[0]);
     final AdEntity goldenEntity = groupEntity[0];
@@ -251,8 +261,7 @@
     adServer.initialize();
 
     groupCatalog.bySid.put("S-1-5-32-users", userGroup);
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     final AdEntity goldenEntity = new AdEntity("S-1-5-32-544",
         "cn=name\\ under,DN_for_default_naming_context", "users", "sam");
@@ -277,9 +286,8 @@
 
     assertTrue(golden.equals(groupCatalog));
 
-    // make sure readFrom call is idempotent
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    // make sure readEverythingFrom call is idempotent
+    groupCatalog.readEverythingFrom(adServer);
     assertTrue(golden.equals(groupCatalog));
   }
 
@@ -306,8 +314,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     final AdEntity goldenEntity = new AdEntity("S-1-5-32-544",
         "cn=name\\ under,DN_for_default_naming_context", "users", "sam");
@@ -330,38 +337,12 @@
 
     assertTrue(golden.equals(groupCatalog));
 
-    // make sure readFrom call is idempotent
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    // make sure readEverythingFrom call is idempotent
+    groupCatalog.readEverythingFrom(adServer);
     assertTrue(golden.equals(groupCatalog));
   }
 
   @Test
-  public void testGetModifiedDocIdsCallsReadFromAllServersCorrectly()
-      throws Exception {
-    FakeAdaptor adAdaptor = new FakeAdaptor() {
-    boolean attemptedIncrementalPush = false;
-      @Override
-      public void readFromAllServers(DocIdPusher pusher,
-          boolean doIncrementalPushIfPossible)
-          throws InterruptedException, IOException {
-        attemptedIncrementalPush = true;
-      }
-      @Override
-      public boolean equals(Object o) {
-        boolean results = attemptedIncrementalPush;
-        attemptedIncrementalPush = false;
-        return results;
-      }
-    };
-
-    assertFalse(adAdaptor.equals(null));
-    adAdaptor.getModifiedDocIds(null);
-    assertTrue(adAdaptor.equals(null));
-    assertFalse(adAdaptor.equals(null));
-  }
-
-  @Test
   public void testFullCrawlVersusIncrementalCrawlFlow() throws Exception {
     FakeAdaptor adAdaptor = new FakeAdaptor();
     FakeCatalog groupCatalog = new FakeCatalog(defaultLocalizedStringMap(),
@@ -372,42 +353,40 @@
     // the following 3 lines initialize AdAdAptor.
     AccumulatingDocIdPusher pusher = new AccumulatingDocIdPusher();
     Map<String, String> configEntries = defaultConfig();
-    pushGroupDefinitions(adAdaptor, configEntries, pusher);
 
     groupCatalog.resetCrawlFlags();
-    assertFalse(groupCatalog.ranFullCrawl);
-    assertFalse(groupCatalog.ranIncrementalCrawl);
+    assertFalse(groupCatalog.ranFullCrawl());
+    assertFalse(groupCatalog.ranIncrementalCrawl());
 
-    groupCatalog.readFrom(adAdaptor.getServer(), false, "ds_service_name",
+    groupCatalog.readEverythingFrom(adServer);
+    assertTrue(groupCatalog.ranFullCrawl());
+    assertFalse(groupCatalog.ranIncrementalCrawl());
+
+    groupCatalog.resetCrawlFlags();
+    groupCatalog.readUpdatesFrom(adServer, "other_ds_service_name",
         "0x0123456789abc", 12345678L);
-    assertTrue(groupCatalog.ranFullCrawl);
-    assertFalse(groupCatalog.ranIncrementalCrawl);
+    assertTrue(groupCatalog.ranFullCrawl());
+    assertFalse(groupCatalog.ranIncrementalCrawl());
 
     groupCatalog.resetCrawlFlags();
-    groupCatalog.readFrom(adAdaptor.getServer(), true, "other_ds_service_name",
-        "0x0123456789abc", 12345678L);
-    assertTrue(groupCatalog.ranFullCrawl);
-    assertFalse(groupCatalog.ranIncrementalCrawl);
-
-    groupCatalog.resetCrawlFlags();
-    groupCatalog.readFrom(adAdaptor.getServer(), true, "ds_service_name",
+    groupCatalog.readUpdatesFrom(adServer, "ds_service_name",
         "otherInvocationId", 12345678L);
-    assertTrue(groupCatalog.ranFullCrawl);
-    assertFalse(groupCatalog.ranIncrementalCrawl);
+    assertTrue(groupCatalog.ranFullCrawl());
+    assertFalse(groupCatalog.ranIncrementalCrawl());
 
     groupCatalog.resetCrawlFlags();
-    groupCatalog.readFrom(adAdaptor.getServer(), true, "ds_service_name",
-        "0x0123456789abc", 12345678L); // last USN as previous run: no crawl
-    assertFalse(groupCatalog.ranFullCrawl);
-    assertFalse(groupCatalog.ranIncrementalCrawl);
+    groupCatalog.readUpdatesFrom(adServer, "ds_service_name", "0x0123456789abc",
+        12345678L); // last USN as previous run: no crawl
+    assertFalse(groupCatalog.ranFullCrawl());
+    assertFalse(groupCatalog.ranIncrementalCrawl());
 
     // call Incremental call (only) when it's desired and the service name and
     // invocation ID both match and the HighestUSN does not match.
     groupCatalog.resetCrawlFlags();
-    groupCatalog.readFrom(adAdaptor.getServer(), true, "ds_service_name",
-        "0x0123456789abc", 12345677L);
-    assertFalse(groupCatalog.ranFullCrawl);
-    assertTrue(groupCatalog.ranIncrementalCrawl);
+    groupCatalog.readUpdatesFrom(adServer, "ds_service_name", "0x0123456789abc",
+        12345677L);
+    assertFalse(groupCatalog.ranFullCrawl());
+    assertTrue(groupCatalog.ranIncrementalCrawl());
   }
 
   @Test
@@ -421,7 +400,7 @@
     String filter = "(|(&(objectClass=group)"
         + "(groupType:1.2.840.113556.1.4.803:=2147483648))"
         + "(&(objectClass=user)(objectCategory=person)))";
-    String incrementalFilter = "(&(uSNChanged>12345677)" + filter + ")";
+    String incrementalFilter = "(&(uSNChanged>=12345678)" + filter + ")";
     String searchDn = "DN_for_default_naming_context";
     List<String> members = Arrays.asList("dn_for_user_1", "dn_for_user_2");
     ldapContext.addSearchResult(filter, "cn", searchDn, "group_name")
@@ -456,12 +435,11 @@
     adServer.initialize();
 
     // first, do a full crawl
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     // now do an incremental crawl
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ true,
-        "ds_service_name", "0x0123456789abc", 12345677L);
+    groupCatalog.readUpdatesFrom(adServer, "ds_service_name", "0x0123456789abc",
+        12345677L);
 
     // extract incrementally-added user as one golden entity
     Set<AdEntity> incrementalUserSet = adServer.search(incrementalFilter, false,
@@ -508,8 +486,6 @@
       /*bySid*/ goldenSid,
       /*byDn*/ goldenDn,
       /*domain*/ goldenDomain);
-
-    assertTrue(golden.equals(groupCatalog));
   }
 
   @Test
@@ -549,8 +525,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     // add two additional entities to test all branches of our method.
     // first -- a user
@@ -632,8 +607,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     tweakGroupCatalogForMakeDefs(groupCatalog, adServer, false);
 
@@ -662,8 +636,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     tweakGroupCatalogForMakeDefs(groupCatalog, adServer, true);
 
@@ -702,8 +675,7 @@
     AdServer adServer = new AdServer("localhost", ldapContext);
     adServer.initialize();
 
-    groupCatalog.readFrom(adServer, /*doIncrementalPushIfPossible=*/ false,
-        "ds_service_name", "0x0123456789abc", 12345678L);
+    groupCatalog.readEverythingFrom(adServer);
 
     tweakGroupCatalogForMakeDefs(groupCatalog, adServer, false);
     // now replace the parent group with a well-known one
@@ -785,7 +757,7 @@
     configEntries.put("ad.defaultPassword", "password");
     configEntries.put("server.port", "5680");
     configEntries.put("server.dashboardPort", "5681");
-    pushGroupDefinitions(adAdaptor, configEntries, pusher);
+    pushGroupDefinitions(adAdaptor, configEntries, pusher, /*fullPush=*/ true);
     Map<GroupPrincipal, Collection<Principal>> results = pusher.getGroups();
     // the above (eventually) calls AdAdaptor.init() with the specified config.
   }
@@ -855,7 +827,7 @@
     AdAdaptor adAdaptor = new FakeAdaptor();
     AccumulatingDocIdPusher pusher = new AccumulatingDocIdPusher();
     Map<String, String> configEntries = defaultConfig();
-    pushGroupDefinitions(adAdaptor, configEntries, pusher);
+    pushGroupDefinitions(adAdaptor, configEntries, pusher, /*fullPush=*/ true);
     Map<GroupPrincipal, Collection<Principal>> results = pusher.getGroups();
 
     final Map<GroupPrincipal, Collection<Principal>> goldenGroups =
@@ -873,7 +845,12 @@
     assertEquals(goldenGroups, results);
 
     // make sure pushGroupDefinitions call is idempotent
-    pushGroupDefinitions(adAdaptor, configEntries, pusher);
+    pushGroupDefinitions(adAdaptor, configEntries, pusher, /*fullPush=*/ true);
+    results = pusher.getGroups();
+    assertEquals(goldenGroups, results);
+
+    // even when doing an incremental push
+    pushGroupDefinitions(adAdaptor, configEntries, pusher, /*fullPush=*/ false);
     results = pusher.getGroups();
     assertEquals(goldenGroups, results);
   }
@@ -913,7 +890,8 @@
     AccumulatingDocIdPusher pusher = new AccumulatingDocIdPusher();
     Map<String, String> configEntries = defaultConfig();
     try {
-      pushGroupDefinitions(adAdaptor, configEntries, pusher);
+      pushGroupDefinitions(adAdaptor, configEntries, pusher,
+          /*fullPush=*/ true);
       fail("Did not catch expected IOException.");
     } catch (IOException ioe) {
       assertTrue(ioe.getCause().getMessage().equals("Catch me if you can!"));
@@ -1062,14 +1040,15 @@
     adaptor.init(TestHelper.createConfigAdaptorContext(config));
   }
 
-  /**
-   * Copied in from TestHelper (from the library)
-   */
-  public static void pushGroupDefinitions(Adaptor adaptor,
-      Map<String, String> configEntries, final DocIdPusher pusher)
-      throws Exception {
+  public static void pushGroupDefinitions(AdAdaptor adaptor,
+      Map<String, String> configEntries, final DocIdPusher pusher,
+      boolean fullPush) throws Exception {
     initializeAdaptorConfig(adaptor, configEntries);
-    adaptor.getDocIds(pusher);
+    if (fullPush) {
+      adaptor.getDocIds(pusher);
+    } else {
+      adaptor.getModifiedDocIds(pusher);
+    }
   }
 
   /**
@@ -1168,7 +1147,6 @@
 
   /** A version of AdAdaptor that uses only mock AdServers */
   public class FakeAdaptor extends AdAdaptor {
-    private AdServer fakeServer = null;
     @Override
     AdServer newAdServer(Method method, String host, int port,
         String principal, String passwd) {
@@ -1178,20 +1156,19 @@
       } catch (Exception e) {
         fail("Could not create LdapContext:" + e);
       }
-<<<<<<< HEAD
-      fakeServer = new AdServer(host, ldapContext);
-      return fakeServer;
-    }
-
-    AdServer getServer() {
-      return fakeServer;
+      return new AdServer(host, ldapContext) {
+        @Override
+        void recreateLdapContext() {
+          // leave ldapContext unchanged
+        }
+      };
     }
   };
 
   /** Simple Fake of GroupCatalog that tracks calls to full/incremental crawl */
-  public class FakeCatalog extends AdAdaptor.GroupCatalog {
-    boolean ranFullCrawl;
-    boolean ranIncrementalCrawl;
+  private static class FakeCatalog extends AdAdaptor.GroupCatalog {
+    private boolean ranFullCrawl;
+    private boolean ranIncrementalCrawl;
 
     public FakeCatalog(Map<String, String> localizedStrings, String namespace,
         boolean feedBuiltinGroups) {
@@ -1199,29 +1176,30 @@
     }
 
     @Override
-    void fullCrawl(AdServer server) throws InterruptedNamingException {
+    void readEverythingFrom(AdServer server) throws InterruptedNamingException {
       ranIncrementalCrawl = false;
       ranFullCrawl = true;
     }
 
     @Override
-    void incrementalCrawl(AdServer server, long previousHighestUSN,
+    Set<AdEntity> incrementalCrawl(AdServer server, long previousHighestUSN,
         long currentHighestUSN) throws InterruptedNamingException {
       ranFullCrawl = false;
       ranIncrementalCrawl = true;
+      return Collections.emptySet();
     }
 
     void resetCrawlFlags() {
       ranFullCrawl = false;
       ranIncrementalCrawl = false;
-=======
-      return new AdServer(host, ldapContext) {
-        @Override
-        void recreateLdapContext() {
-          // leave ldapContext unchanged
-        }
-      };
->>>>>>> 5a3b41cfed0457a17ed22df6736176fc3469ee0c
+    }
+
+    public boolean ranIncrementalCrawl() {
+      return ranIncrementalCrawl;
+    }
+
+    public boolean ranFullCrawl() {
+      return ranFullCrawl;
     }
   }
 }