6007 scheduler id problems with multiple instances (#6012)

* Modify cluster instance name

* spotless

* Add test for scheduler name

* Changelog

* Remove atomic int for autogenerating IDs

* Remove atomic int for autogenerating IDs

* Fix test

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6007-instance-name-sharing.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 6007
+title: "Previously, HAPI-FHIR schedulers were automatically suffixing the Quartz scheduler's `org.quartz.scheduler.instanceName` value to a unique autogenerated value based on the provided instance name. This
+has been corrected, and now the instance name provided to the BaseHapiFhirScheduler is used directly, with no unique suffix added. This is in line with the [Quartz Documentation](https://www.quartz-scheduler.org/documentation/quartz-2.1.7/configuration/ConfigMain.html) related
+to defining the scheduler instance name for those who run schedulers in a cluster."

hapi-fhir-jpa/src/main/java/ca/uhn/fhir/jpa/sched/BaseHapiScheduler.java

@@ -48,7 +48,6 @@ import org.springframework.scheduling.quartz.SchedulerFactoryBean;
 import java.util.List;
 import java.util.Properties;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
@@ -56,8 +55,6 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank;
 public abstract class BaseHapiScheduler implements IHapiScheduler {
 	private static final Logger ourLog = LoggerFactory.getLogger(BaseHapiScheduler.class);
 
-	private static final AtomicInteger ourNextSchedulerId = new AtomicInteger();
-
 	private final String myThreadNamePrefix;
 	private final AutowiringSpringBeanJobFactory mySpringBeanJobFactory;
 	private final SchedulerFactoryBean myFactory = new SchedulerFactoryBean();
@@ -75,18 +72,16 @@ public abstract class BaseHapiScheduler implements IHapiScheduler {
 		myInstanceName = theInstanceName;
 	}
 
-	int nextSchedulerId() {
-		return ourNextSchedulerId.getAndIncrement();
-	}
-
 	@Override
 	public void init() throws SchedulerException {
+
 		setProperties();
 		myFactory.setQuartzProperties(myProperties);
 		myFactory.setBeanName(myInstanceName);
 		myFactory.setSchedulerName(myThreadNamePrefix);
 		myFactory.setJobFactory(mySpringBeanJobFactory);
 		massageJobFactory(myFactory);
+
 		try {
 			Validate.notBlank(myInstanceName, "No instance name supplied");
 			myFactory.afterPropertiesSet();
@@ -104,8 +99,17 @@ public abstract class BaseHapiScheduler implements IHapiScheduler {
 
 	protected void setProperties() {
 		addProperty("org.quartz.threadPool.threadCount", "4");
-		myProperties.setProperty(
-				StdSchedulerFactory.PROP_SCHED_INSTANCE_NAME, myInstanceName + "-" + nextSchedulerId());
+		// Note that we use a common name, with no suffixed ID for the name, as per the quartz docs:
+		// https://www.quartz-scheduler.org/documentation/quartz-2.1.7/configuration/ConfigMain.html
+		if (myInstanceName != null) {
+			myProperties.setProperty(StdSchedulerFactory.PROP_SCHED_INSTANCE_NAME, myInstanceName);
+		}
+
+		// By Default, the scheduler ID is not set, which will cause quartz to set it to the string NON_CLUSTERED. Here
+		// we are setting it explicitly as an indication to implementers that if they want a different ID, they should
+		// set it using this below property.
+		addProperty(StdSchedulerFactory.PROP_SCHED_INSTANCE_ID, StdSchedulerFactory.DEFAULT_INSTANCE_ID);
+
 		addProperty("org.quartz.threadPool.threadNamePrefix", getThreadPrefix());
 	}
 
@@ -284,4 +288,14 @@ public abstract class BaseHapiScheduler implements IHapiScheduler {
 			ourLog.error("Error triggering scheduled job with key {}", theJobDefinition);
 		}
 	}
+
+	/**
+	 * Retrieves a clone of the properties required for unit testing.
+	 *
+	 * @return The properties for unit testing.
+	 */
+	@VisibleForTesting
+	protected Properties getPropertiesForUnitTest() {
+		return myProperties;
+	}
 }

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/sched/BaseHapiSchedulerTest.java

@@ -3,7 +3,9 @@ package ca.uhn.fhir.jpa.sched;
 import ca.uhn.fhir.i18n.Msg;
 import org.junit.jupiter.api.Test;
 import org.quartz.SchedulerException;
+import org.quartz.impl.StdSchedulerFactory;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -21,5 +23,29 @@ public class BaseHapiSchedulerTest {
 		}
 	}
 
+	@Test
+	public void testSchedulersShareTheSameServiceName() throws SchedulerException {
+
+		String instanceName = "local-scheduler";
+		String instanceID = "NON_CLUSTERED";
+
+		BaseHapiScheduler firstScheduler = new BaseHapiScheduler("hello", new AutowiringSpringBeanJobFactory()) {
+		};
+		firstScheduler.setInstanceName(instanceName);
+
+		BaseHapiScheduler secondScheduler = new BaseHapiScheduler("hello", new AutowiringSpringBeanJobFactory()) {
+		};
+		secondScheduler.setInstanceName(instanceName);
+
+		firstScheduler.init();
+		secondScheduler.init();
+
+		assertThat(firstScheduler.getPropertiesForUnitTest()).containsEntry(StdSchedulerFactory.PROP_SCHED_INSTANCE_NAME, instanceName);
+		assertThat(secondScheduler.getPropertiesForUnitTest()).containsEntry(StdSchedulerFactory.PROP_SCHED_INSTANCE_NAME, instanceName);
+
+		assertThat(firstScheduler.getPropertiesForUnitTest()).containsEntry(StdSchedulerFactory.PROP_SCHED_INSTANCE_ID, instanceID);
+		assertThat(secondScheduler.getPropertiesForUnitTest()).containsEntry(StdSchedulerFactory.PROP_SCHED_INSTANCE_ID, instanceID);
+	}
+
 
 }