[ SEARCH-1752 ] Review comments addressed

search-services/alfresco-search/src/main/java/org/alfresco/solr/lifecycle/SolrCoreLoadListener.java

@@ -18,6 +18,7 @@
  */
 package org.alfresco.solr.lifecycle;
 
+import static java.util.Arrays.asList;
 import static java.util.Optional.ofNullable;
 
 import org.alfresco.opencmis.dictionary.CMISStrictDictionaryService;
@@ -220,8 +221,6 @@ public class SolrCoreLoadListener extends AbstractSolrEventListener
                 core.getName(),
                 core.hashCode());
 
-        LOGGER.info("SearchServices Core Trackers have been correctly registered and scheduled on Solr Core instance {} with name {}", core.hashCode(), core.getName());
-
         //Add the commitTracker to the list of scheduled trackers that can be shutdown
         trackers.add(commitTracker);
     }
@@ -233,58 +232,68 @@ public class SolrCoreLoadListener extends AbstractSolrEventListener
                                                 SOLRAPIClient repositoryClient,
                                                 SolrInformationServer srv)
     {
-        List<Tracker> trackers = new ArrayList<>();
+        AclTracker aclTracker =
-        String coreName = core.getName();
+                registerAndSchedule(
+                        new AclTracker(props, repositoryClient, core.getName(), srv),
+                        core,
+                        props,
+                        trackerRegistry,
+                        scheduler);
 
-        AclTracker aclTracker = new AclTracker(props, repositoryClient, coreName, srv);
+        ContentTracker contentTracker =
-        trackerRegistry.register(coreName, aclTracker);
+                registerAndSchedule(
-        scheduler.schedule(aclTracker, coreName, props);
+                    new ContentTracker(props, repositoryClient, core.getName(), srv),
+                        core,
+                        props,
+                        trackerRegistry,
+                        scheduler);
 
-        LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.",
+        MetadataTracker metadataTracker =
-                aclTracker.getClass().getSimpleName(),
+                registerAndSchedule(
-                aclTracker.hashCode(),
+                    new MetadataTracker(true, props, repositoryClient, core.getName(), srv),
-                core.getName(),
+                        core,
-                core.hashCode());
+                        props,
+                        trackerRegistry,
+                        scheduler);
 
-        ContentTracker contentTrkr = new ContentTracker(props, repositoryClient, coreName, srv);
+        CascadeTracker cascadeTracker =
-        trackerRegistry.register(coreName, contentTrkr);
+                registerAndSchedule(
-        scheduler.schedule(contentTrkr, coreName, props);
+                    new CascadeTracker(props, repositoryClient, core.getName(), srv),
-
+                        core,
-        LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.",
+                        props,
-                contentTrkr.getClass().getSimpleName(),
+                        trackerRegistry,
-                contentTrkr.hashCode(),
+                        scheduler);
-                core.getName(),
-                core.hashCode());
-
-        MetadataTracker metaTrkr = new MetadataTracker(true, props, repositoryClient, coreName, srv);
-        trackerRegistry.register(coreName, metaTrkr);
-        scheduler.schedule(metaTrkr, coreName, props);
-
-        LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.",
-                metaTrkr.getClass().getSimpleName(),
-                metaTrkr.hashCode(),
-                core.getName(),
-                core.hashCode());
-
-        CascadeTracker cascadeTrkr = new CascadeTracker(props, repositoryClient, coreName, srv);
-        trackerRegistry.register(coreName, cascadeTrkr);
-        scheduler.schedule(cascadeTrkr, coreName, props);
-
-        LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.",
-                cascadeTrkr.getClass().getSimpleName(),
-                cascadeTrkr.hashCode(),
-                core.getName(),
-                core.hashCode());
 
         //The CommitTracker will acquire these locks in order
         //The ContentTracker will likely have the longest runs so put it first to ensure the MetadataTracker is not paused while
         //waiting for the ContentTracker to release it's lock.
         //The aclTracker will likely have the shortest runs so put it last.
-        trackers.add(cascadeTrkr);
+        return asList(cascadeTracker, contentTracker, metadataTracker, aclTracker);
-        trackers.add(contentTrkr);
+    }
-        trackers.add(metaTrkr);
+
-        trackers.add(aclTracker);
+    /**
-        return trackers;
+     * Accepts a {@link Tracker} instance, registers and schedules it.
+     *
+     * @param tracker the tracker that will be scheduled and registered.
+     * @param core the owning core.
+     * @param properties configuration properties.
+     * @param registry the tracker registry instance.
+     * @param scheduler the tracker schedule instance.
+     * @param <T> the tracker instance.
+     * @return the registered and scheduled tracker instance.
+     */
+    private <T extends Tracker> T registerAndSchedule(T tracker, SolrCore core, Properties properties, TrackerRegistry registry, SolrTrackerScheduler scheduler)
+    {
+        registry.register(core.getName(), tracker);
+        scheduler.schedule(tracker, core.getName(), properties);
+
+        LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.",
+                tracker.getClass().getSimpleName(),
+                tracker.hashCode(),
+                core.getName(),
+                core.hashCode());
+
+        return tracker;
     }
 
     private void createModelTracker(String coreName,
@@ -353,35 +362,22 @@ public class SolrCoreLoadListener extends AbstractSolrEventListener
      */
     private void shutdownTracker(SolrCore core, Tracker tracker, SolrTrackerScheduler scheduler, boolean coreHasBeenReloaded)
     {
+        // In case of reload the input core is not the owner: the owner is instead the previous (closed) core and we don't have its reference here.
+        String coreReference = core.getName() + (coreHasBeenReloaded ? "" : ", instance " + core.hashCode());
+
         if (tracker.isAlreadyInShutDownMode())
         {
-            LOGGER.info("Tracker {}, instance {} belonging to core {}, instance {}, is already in shutdown mode.",
+            LOGGER.info("Tracker {}, instance {} belonging to core {}, is already in shutdown mode.",
                     tracker.getClass().getSimpleName(),
                     tracker.hashCode(),
-                    core.getName(),
+                    coreReference);
-                    core.hashCode());
             return;
         }
 
-        // Just differentiate the log message: in case of reload the input core is not the owner: the owner
+        LOGGER.info("Tracker {}, instance {} belonging to core {} shutdown procedure initiated.",
-        // is instead the previous (closed) core and we don't have its reference here.
-        if (coreHasBeenReloaded)
-        {
-            LOGGER.info("Tracker {}, instance {} belonging to core {} shutdown procedure initiated." +
-                            " If you reloaded the core that is probably the reason you're seeing this message.",
                 tracker.getClass().getSimpleName(),
                 tracker.hashCode(),
-                    core.getName());
+                coreReference);
-        }
-        else
-        {
-            LOGGER.info("Tracker {}, instance {} belonging to core {}, instance {}, shutdown procedure initiated.",
-                    tracker.getClass().getSimpleName(),
-                    tracker.hashCode(),
-                    core.getName(),
-                    core.hashCode());
-        }
-
         try
         {
             tracker.setShutdown(true);
@@ -392,40 +388,19 @@ public class SolrCoreLoadListener extends AbstractSolrEventListener
 
             tracker.shutdown();
 
-            if (coreHasBeenReloaded)
-            {
             LOGGER.info("Tracker {}, instance {}, belonging to core {} shutdown procedure correctly terminated.",
                     tracker.getClass().getSimpleName(),
                     tracker.hashCode(),
-                        core.getName());
+                    coreReference);
         }
-            else
+        catch (Exception exception)
         {
-                LOGGER.info("Tracker {}, instance {} belonging to core {}, instance {}, shutdown procedure correctly terminated.",
+            LOGGER.error("Tracker {}, instance {} belonging to core {}, shutdown procedure failed. " +
-                        tracker.getClass().getSimpleName(),
-                        tracker.hashCode(),
-                        core.getName(),
-                        core.hashCode());
-            }
-        }
-        catch (Exception e)
-        {
-            if (coreHasBeenReloaded)
-            {
-                LOGGER.info("Tracker {}, instance {} belonging to core {} shutdown procedure correctly terminated.",
-                        tracker.getClass().getSimpleName(),
-                        tracker.hashCode(),
-                        core.getName());
-            }
-            else
-            {
-                LOGGER.error("Tracker {}, instance {} belonging to core {}, instance {}, shutdown procedure failed. " +
                             "See the stacktrace below for further details.",
                     tracker.getClass().getSimpleName(),
                     tracker.hashCode(),
-                        core.getName(),
+                    coreReference,
-                        core.hashCode(), e);
+                    exception);
-            }
         }
     }
 

search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/AbstractTracker.java

@@ -18,6 +18,7 @@
  */
 package org.alfresco.solr.tracker;
 
+import java.lang.invoke.MethodHandles;
 import java.net.ConnectException;
 import java.net.SocketTimeoutException;
 import java.util.Properties;
@@ -41,7 +42,8 @@ public abstract class AbstractTracker implements Tracker
     static final long TIME_STEP_32_DAYS_IN_MS = 1000 * 60 * 60 * 24 * 32L;
     static final long TIME_STEP_1_HR_IN_MS = 60 * 60 * 1000L;
     static final String SHARD_METHOD_DBID = "DB_ID";
-    protected final static Logger log = LoggerFactory.getLogger(AbstractTracker.class);
+
+    protected final static Logger LOGGER = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
     
     protected Properties props;    
     protected SOLRAPIClient client;
@@ -105,7 +107,7 @@ public abstract class AbstractTracker implements Tracker
         
         this.type = type;
         
-        log.info("Solr built for Alfresco version: " + alfrescoVersion);
+        LOGGER.info("Solr built for Alfresco version: " + alfrescoVersion);
     }
 
     
@@ -164,7 +166,7 @@ public abstract class AbstractTracker implements Tracker
     public void track()
     {
         if(runLock.availablePermits() == 0) {
-            log.info("... " + this.getClass().getSimpleName() + " for core [" + coreName + "] is already in use "+ this.getClass());
+            LOGGER.info("... " + this.getClass().getSimpleName() + " for core [" + coreName + "] is already in use "+ this.getClass());
             return;
         }
 
@@ -182,7 +184,7 @@ public abstract class AbstractTracker implements Tracker
                 assert(assertTrackerStateRemainsNull());
             }
 
-            log.info("... Running " + this.getClass().getSimpleName() + " for core [" + coreName + "].");
+            LOGGER.info("... Running " + this.getClass().getSimpleName() + " for core [" + coreName + "].");
             
             if(this.state == null)
             {
@@ -190,8 +192,8 @@ public abstract class AbstractTracker implements Tracker
                 * Set the global state for the tracker here.
                 */
                 this.state = getTrackerState();
-                log.debug("##### Setting tracker global state.");
+                LOGGER.debug("##### Setting tracker global state.");
-                log.debug("State set: " + this.state.toString());
+                LOGGER.debug("State set: " + this.state.toString());
                 this.state.setRunning(true);
             }
             else
@@ -209,33 +211,33 @@ public abstract class AbstractTracker implements Tracker
             catch(IndexTrackingShutdownException t)
             {
                 setRollback(true);
-                log.info("Stopping index tracking for " + getClass().getSimpleName() + " - " + coreName);
+                LOGGER.info("Stopping index tracking for " + getClass().getSimpleName() + " - " + coreName);
             }
             catch(Throwable t)
             {
                 setRollback(true);
                 if (t instanceof SocketTimeoutException || t instanceof ConnectException)
                 {
-                    if (log.isDebugEnabled())
+                    if (LOGGER.isDebugEnabled())
                     {
                         // DEBUG, so give the whole stack trace
-                        log.warn("Tracking communication timed out for " + getClass().getSimpleName() + " - " + coreName, t);
+                        LOGGER.warn("Tracking communication timed out for " + getClass().getSimpleName() + " - " + coreName, t);
                     }
                     else
                     {
                         // We don't need the stack trace.  It timed out.
-                        log.warn("Tracking communication timed out for " + getClass().getSimpleName() + " - " + coreName);
+                        LOGGER.warn("Tracking communication timed out for " + getClass().getSimpleName() + " - " + coreName);
                     }
                 }
                 else
                 {
-                    log.error("Tracking failed for " + getClass().getSimpleName() + " - " + coreName, t);
+                    LOGGER.error("Tracking failed for " + getClass().getSimpleName() + " - " + coreName, t);
                 }
             }
         }
         catch (InterruptedException e)
         {
-            log.error("Semaphore interrupted for " + getClass().getSimpleName() + " - " + coreName, e);
+            LOGGER.error("Semaphore interrupted for " + getClass().getSimpleName() + " - " + coreName, e);
         }
         finally
         {

search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/MetadataTracker.java

@@ -49,7 +49,7 @@ import org.slf4j.LoggerFactory;
  * This tracks two things: transactions and metadata nodes
  * @author Ahmed Owian
  */
-public class MetadataTracker extends NodeStateProvider implements Tracker
+public class MetadataTracker extends NodeStatePublisher implements Tracker
 {
     protected final static Logger log = LoggerFactory.getLogger(MetadataTracker.class);
     private static final int DEFAULT_TRANSACTION_DOCS_BATCH_SIZE = 100;

search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/ModelTracker.java

@@ -106,7 +106,7 @@ public class ModelTracker extends AbstractTracker implements Tracker
         super(p, client, coreName, informationServer, Tracker.Type.MODEL);
         String normalSolrHome = SolrResourceLoader.normalizeDir(solrHome);
         alfrescoModelDir = new File(ConfigUtil.locateProperty("solr.model.dir", normalSolrHome+"alfrescoModels"));
-        log.info("Alfresco Model dir " + alfrescoModelDir);
+        LOGGER.info("Alfresco Model dir " + alfrescoModelDir);
         if (!alfrescoModelDir.exists())
         {
             alfrescoModelDir.mkdir();
@@ -197,7 +197,7 @@ public class ModelTracker extends AbstractTracker implements Tracker
         int registeredSearcherCount = this.infoSrv.getRegisteredSearcherCount();
         if (registeredSearcherCount >= getMaxLiveSearchers())
         {
-            log.info(".... skipping tracking registered searcher count = " + registeredSearcherCount);
+            LOGGER.info(".... skipping tracking registered searcher count = " + registeredSearcherCount);
             return;
         }
 
@@ -268,7 +268,7 @@ public class ModelTracker extends AbstractTracker implements Tracker
         }
         catch (Throwable t)
         {
-            log.error("Model tracking failed for core: "+ coreName, t);
+            LOGGER.error("Model tracking failed for core: "+ coreName, t);
         }
 
     }
@@ -534,7 +534,7 @@ public class ModelTracker extends AbstractTracker implements Tracker
             {
                 loadedModels.add(modelName);
             }
-            log.info("Loading model " + model.getName());
+            LOGGER.info("Loading model " + model.getName());
         }
     }
 

search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStatePublisher.java

@@ -59,7 +59,7 @@ import java.util.Properties;
  * @since 1.5
  * @see <a href="https://issues.alfresco.com/jira/browse/SEARCH-1752">SEARCH-1752</a>
  */
-public abstract class NodeStateProvider extends AbstractTracker
+public abstract class NodeStatePublisher extends AbstractTracker
 {
     DocRouter docRouter;
     private final boolean isMaster;
@@ -70,7 +70,7 @@ public abstract class NodeStateProvider extends AbstractTracker
     /** The property to use for determining the shard. */
     protected Optional<QName> shardProperty = Optional.empty();
 
-    NodeStateProvider(
+    NodeStatePublisher(
             boolean isMaster,
             Properties p,
             SOLRAPIClient client,
@@ -88,7 +88,7 @@ public abstract class NodeStateProvider extends AbstractTracker
         docRouter = DocRouterFactory.getRouter(p, ShardMethodEnum.getShardMethod(shardMethod));
     }
 
-    NodeStateProvider(Type type)
+    NodeStatePublisher(Type type)
     {
         super(type);
         this.isMaster = false;
@@ -100,7 +100,7 @@ public abstract class NodeStateProvider extends AbstractTracker
             updateShardProperty();
             if (shardProperty.isEmpty())
             {
-                log.warn("Sharding property " + SHARD_KEY_KEY + " was set to " + shardKeyName + ", but no such property was found.");
+                LOGGER.warn("Sharding property {} was set to {}, but no such property was found.", SHARD_KEY_KEY, shardKeyName);
             }
         });
     }
@@ -116,11 +116,11 @@ public abstract class NodeStateProvider extends AbstractTracker
             {
                 if (updatedShardProperty.isEmpty())
                 {
-                    log.warn("The model defining " + shardKeyName + " property has been disabled");
+                    LOGGER.warn("The model defining {} property has been disabled", shardKeyName);
                 }
                 else
                 {
-                    log.info("New " + SHARD_KEY_KEY + " property found for " + shardKeyName);
+                    LOGGER.info("New {} property found for {} ", SHARD_KEY_KEY, shardKeyName);
                 }
             }
             shardProperty = updatedShardProperty;
@@ -148,11 +148,8 @@ public abstract class NodeStateProvider extends AbstractTracker
                 namespaceDAO,
                 dictionaryService,
                 field);
-        if (propertyDef == null)
+
-        {
+        return ofNullable(propertyDef).map(PropertyDefinition::getName);
-            return Optional.empty();
-        }
-        return of(propertyDef.getName());
     }
 
     /**

search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SlaveNodeStatePublisher.java

@@ -27,7 +27,7 @@ import java.util.Properties;
  * @author Andrea Gazzarini
  * @since 1.5
  */
-public class SlaveNodeStatePublisher extends NodeStateProvider
+public class SlaveNodeStatePublisher extends NodeStatePublisher
 {
     public SlaveNodeStatePublisher(
             boolean isMaster,
@@ -47,24 +47,12 @@ public class SlaveNodeStatePublisher extends NodeStateProvider
             ShardState shardstate = getShardState();
             client.getTransactions(0L, null, 0L, null, 0, shardstate);
         }
-        catch (EncoderException exception )
+        catch (EncoderException | IOException | AuthenticationException exception )
         {
-            log.error("Unable to publish this node state. " +
+            LOGGER.error("Unable to publish this node state. " +
                     "A failure condition has been met during the outbound subscription message encoding process. " +
                     "See the stacktrace below for further details.", exception);
         }
-        catch (IOException exception )
-        {
-            log.error("Unable to publish this node state. " +
-                    "Detected an I/O failure while sending the subscription state message. " +
-                    "See the stacktrace below for further details.", exception);
-        }
-        catch (AuthenticationException exception)
-        {
-            log.error("Unable to publish this node state. " +
-                    "Authentication failure detected while sending the subscription state message. " +
-                    "See the stacktrace below for further details.", exception);
-        }
     }
 
     @Override