From 25bf4a69c2f3950859c9a805ca4335467aae038c Mon Sep 17 00:00:00 2001 From: agazzarini Date: Wed, 30 Oct 2019 17:32:06 +0100 Subject: [PATCH] [ SEARCH-1752 ] Review comments addressed --- .../solr/lifecycle/SolrCoreLoadListener.java | 175 ++++++++---------- .../solr/tracker/AbstractTracker.java | 26 +-- .../solr/tracker/MetadataTracker.java | 2 +- .../alfresco/solr/tracker/ModelTracker.java | 8 +- ...eProvider.java => NodeStatePublisher.java} | 19 +- .../solr/tracker/SlaveNodeStatePublisher.java | 18 +- .../solr/tracker/SolrTrackerScheduler.java | 2 +- 7 files changed, 106 insertions(+), 144 deletions(-) rename search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/{NodeStateProvider.java => NodeStatePublisher.java} (93%) diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/lifecycle/SolrCoreLoadListener.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/lifecycle/SolrCoreLoadListener.java index 666255ec5..0b41cba15 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/lifecycle/SolrCoreLoadListener.java +++ b/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 trackers = new ArrayList<>(); - String coreName = core.getName(); + AclTracker aclTracker = + registerAndSchedule( + new AclTracker(props, repositoryClient, core.getName(), srv), + core, + props, + trackerRegistry, + scheduler); - AclTracker aclTracker = new AclTracker(props, repositoryClient, coreName, srv); - trackerRegistry.register(coreName, aclTracker); - scheduler.schedule(aclTracker, coreName, props); + ContentTracker contentTracker = + registerAndSchedule( + new ContentTracker(props, repositoryClient, core.getName(), srv), + core, + props, + trackerRegistry, + scheduler); - LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.", - aclTracker.getClass().getSimpleName(), - aclTracker.hashCode(), - core.getName(), - core.hashCode()); + MetadataTracker metadataTracker = + registerAndSchedule( + new MetadataTracker(true, props, repositoryClient, core.getName(), srv), + core, + props, + trackerRegistry, + scheduler); - ContentTracker contentTrkr = new ContentTracker(props, repositoryClient, coreName, srv); - trackerRegistry.register(coreName, contentTrkr); - scheduler.schedule(contentTrkr, coreName, props); - - LOGGER.info("Tracker {}, instance {}, belonging to Core {}, instance {} has been registered and scheduled.", - contentTrkr.getClass().getSimpleName(), - contentTrkr.hashCode(), - 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()); + CascadeTracker cascadeTracker = + registerAndSchedule( + new CascadeTracker(props, repositoryClient, core.getName(), srv), + core, + props, + trackerRegistry, + scheduler); //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); - trackers.add(contentTrkr); - trackers.add(metaTrkr); - trackers.add(aclTracker); - return trackers; + return asList(cascadeTracker, contentTracker, metadataTracker, aclTracker); + } + + /** + * 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 the tracker instance. + * @return the registered and scheduled tracker instance. + */ + private 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(), - core.hashCode()); + coreReference); return; } - // Just differentiate the log message: 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. - 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()); - } - else - { - LOGGER.info("Tracker {}, instance {} belonging to core {}, instance {}, shutdown procedure initiated.", - tracker.getClass().getSimpleName(), - tracker.hashCode(), - core.getName(), - core.hashCode()); - } - + LOGGER.info("Tracker {}, instance {} belonging to core {} shutdown procedure initiated.", + tracker.getClass().getSimpleName(), + tracker.hashCode(), + coreReference); 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()); - } - else - { - LOGGER.info("Tracker {}, instance {} belonging to core {}, instance {}, shutdown procedure correctly terminated.", - tracker.getClass().getSimpleName(), - tracker.hashCode(), - core.getName(), - core.hashCode()); - } + LOGGER.info("Tracker {}, instance {}, belonging to core {} shutdown procedure correctly terminated.", + tracker.getClass().getSimpleName(), + tracker.hashCode(), + coreReference); } - catch (Exception e) + catch (Exception exception) { - 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(), - core.hashCode(), e); - } + LOGGER.error("Tracker {}, instance {} belonging to core {}, shutdown procedure failed. " + + "See the stacktrace below for further details.", + tracker.getClass().getSimpleName(), + tracker.hashCode(), + coreReference, + exception); } } diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/AbstractTracker.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/AbstractTracker.java index 116408cfe..e32e64636 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/AbstractTracker.java +++ b/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."); - log.debug("State set: " + this.state.toString()); + LOGGER.debug("##### Setting tracker global state."); + 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 { diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/MetadataTracker.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/MetadataTracker.java index 36d092d7a..f118c523b 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/MetadataTracker.java +++ b/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; diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/ModelTracker.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/ModelTracker.java index b3f3cd26b..034d14228 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/ModelTracker.java +++ b/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()); } } diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStateProvider.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStatePublisher.java similarity index 93% rename from search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStateProvider.java rename to search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStatePublisher.java index c64e03d17..d6fd2d78a 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/NodeStateProvider.java +++ b/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 SEARCH-1752 */ -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 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 Optional.empty(); - } - return of(propertyDef.getName()); + + return ofNullable(propertyDef).map(PropertyDefinition::getName); } /** diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SlaveNodeStatePublisher.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SlaveNodeStatePublisher.java index b013328d8..4e7ccf709 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SlaveNodeStatePublisher.java +++ b/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 diff --git a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SolrTrackerScheduler.java b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SolrTrackerScheduler.java index 2038091c5..c584d80fb 100644 --- a/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SolrTrackerScheduler.java +++ b/search-services/alfresco-search/src/main/java/org/alfresco/solr/tracker/SolrTrackerScheduler.java @@ -121,7 +121,7 @@ public class SolrTrackerScheduler case COMMIT: cron = getCron(props,"alfresco.commit.tracker.cron"); break; - case NODE_STATE_PUBLISHER: + case NODE_STATE_PUBLISHER: cron = getCron(props,"alfresco.nodestate.tracker.cron"); break; default: