From 6b9cd9de97c3a80a4f30418592413ec7327fbec5 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Fri, 22 Sep 2017 11:42:29 +0200 Subject: [PATCH] WIP SONAR-9814 make ThreadLocalSettings support DB connectivity issues --- .../server/setting/ThreadLocalSettings.java | 35 +++++- .../setting/ThreadLocalSettingsTest.java | 111 +++++++++++++++++- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java index c2992be406e..a12c4963e02 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java +++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ThreadLocalSettings.java @@ -21,11 +21,13 @@ package org.sonar.server.setting; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Properties; +import org.apache.ibatis.exceptions.PersistenceException; import org.sonar.api.CoreProperties; import org.sonar.api.ce.ComputeEngineSide; import org.sonar.api.config.Encryption; @@ -56,6 +58,8 @@ public class ThreadLocalSettings extends Settings { private final Properties systemProps; private static final ThreadLocal> CACHE = new ThreadLocal<>(); + private Map getPropertyDbFailureCache = Collections.emptyMap(); + private Map getPropertiesDbFailureCache = Collections.emptyMap(); private SettingLoader settingLoader; public ThreadLocalSettings(PropertyDefinitions definitions, Properties props) { @@ -97,7 +101,7 @@ public class ThreadLocalSettings extends Settings { Map dbProps = CACHE.get(); // caching is disabled if (dbProps == null) { - return Optional.ofNullable(settingLoader.load(key)); + return Optional.ofNullable(load(key)); } String loadedValue; @@ -108,12 +112,20 @@ public class ThreadLocalSettings extends Settings { } else { // cache the effective value (null if the property // is not persisted) - loadedValue = settingLoader.load(key); + loadedValue = load(key); dbProps.put(key, loadedValue); } return Optional.ofNullable(loadedValue); } + private String load(String key) { + try { + return settingLoader.load(key); + } catch (PersistenceException e) { + return getPropertyDbFailureCache.get(key); + } + } + @Override protected void set(String key, String value) { requireNonNull(key, "key can't be null"); @@ -147,14 +159,29 @@ public class ThreadLocalSettings extends Settings { * Clears the cache specific to the current thread (if any). */ public void unload() { + Map settings = CACHE.get(); CACHE.remove(); + // update cache of settings to be used in case of DB connectivity error + this.getPropertyDbFailureCache = settings; } @Override public Map getProperties() { ImmutableMap.Builder builder = ImmutableMap.builder(); - settingLoader.loadAll(builder); - systemProps.entrySet().forEach(entry -> builder.put((String) entry.getKey(), (String) entry.getValue())); + loadAll(builder); + systemProps.forEach((key, value) -> builder.put((String) key, (String) value)); return builder.build(); } + + private void loadAll(ImmutableMap.Builder builder) { + try { + ImmutableMap.Builder cacheBuilder = ImmutableMap.builder(); + settingLoader.loadAll(cacheBuilder); + Map cache = cacheBuilder.build(); + builder.putAll(cache); + getPropertiesDbFailureCache = cache; + } catch (PersistenceException e) { + builder.putAll(getPropertiesDbFailureCache); + } + } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java index c5bbbcb9e6e..9dc3e4df4e7 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ThreadLocalSettingsTest.java @@ -27,6 +27,7 @@ import java.util.Map; import java.util.Properties; import java.util.concurrent.CountDownLatch; import javax.annotation.Nullable; +import org.apache.ibatis.exceptions.PersistenceException; import org.junit.After; import org.junit.Rule; import org.junit.Test; @@ -35,8 +36,12 @@ import org.junit.rules.TemporaryFolder; import org.sonar.api.config.PropertyDefinitions; import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.data.MapEntry.entry; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; public class ThreadLocalSettingsTest { @@ -45,12 +50,10 @@ public class ThreadLocalSettingsTest { @Rule public ExpectedException expectedException = ExpectedException.none(); - - private MapSettingLoader dbSettingLoader = new MapSettingLoader(); - @Rule public TemporaryFolder temp = new TemporaryFolder(); + private MapSettingLoader dbSettingLoader = new MapSettingLoader(); private ThreadLocalSettings underTest = null; @After @@ -166,6 +169,25 @@ public class ThreadLocalSettingsTest { assertThat(underTest.getProperties()).containsOnly(entry("system", "from system"), entry("db", "from db"), entry("empty", "")); } + @Test + public void getProperties_is_not_cached_in_thread_cache() { + insertPropertyIntoDb("foo", "bar"); + underTest = create(Collections.emptyMap()); + underTest.load(); + + assertThat(underTest.getProperties()) + .containsOnly(entry("foo", "bar")); + + insertPropertyIntoDb("foo2", "bar2"); + assertThat(underTest.getProperties()) + .containsOnly(entry("foo", "bar"), entry("foo2", "bar2")); + + underTest.unload(); + + assertThat(underTest.getProperties()) + .containsOnly(entry("foo", "bar"), entry("foo2", "bar2")); + } + @Test public void load_creates_a_thread_specific_cache() throws InterruptedException { insertPropertyIntoDb(A_KEY, "v1"); @@ -233,6 +255,89 @@ public class ThreadLocalSettingsTest { underTest.unload(); } + @Test + public void getProperties_return_empty_if_DB_error_on_first_call_ever_out_of_thread_cache() { + SettingLoader settingLoaderMock = mock(SettingLoader.class); + PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB"); + doThrow(toBeThrown).when(settingLoaderMock).loadAll(any(ImmutableMap.Builder.class)); + underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock); + + assertThat(underTest.getProperties()) + .isEmpty(); + } + + @Test + public void getProperties_returns_empty_if_DB_error_on_first_call_ever_in_thread_cache() { + SettingLoader settingLoaderMock = mock(SettingLoader.class); + PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB"); + doThrow(toBeThrown).when(settingLoaderMock).loadAll(any(ImmutableMap.Builder.class)); + underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock); + underTest.load(); + + assertThat(underTest.getProperties()) + .isEmpty(); + } + + @Test + public void getProperties_return_properties_from_previous_thread_cache_if_DB_error_on_not_first_call() { + String key = randomAlphanumeric(3); + String value1 = randomAlphanumeric(4); + String value2 = randomAlphanumeric(5); + SettingLoader settingLoaderMock = mock(SettingLoader.class); + PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB"); + doAnswer(invocationOnMock -> { + ImmutableMap.Builder builder = (ImmutableMap.Builder) invocationOnMock.getArguments()[0]; + builder.put(key, value1); + return null; + }).doThrow(toBeThrown) + .doAnswer(invocationOnMock -> { + ImmutableMap.Builder builder = (ImmutableMap.Builder) invocationOnMock.getArguments()[0]; + builder.put(key, value2); + return null; + }) + .when(settingLoaderMock) + .loadAll(any(ImmutableMap.Builder.class)); + underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock); + + underTest.load(); + assertThat(underTest.getProperties()) + .containsOnly(entry(key, value1)); + underTest.unload(); + + underTest.load(); + assertThat(underTest.getProperties()) + .containsOnly(entry(key, value1)); + underTest.unload(); + + underTest.load(); + assertThat(underTest.getProperties()) + .containsOnly(entry(key, value2)); + underTest.unload(); + } + + @Test + public void get_returns_empty_if_DB_error_on_first_call_ever_out_of_thread_cache() { + SettingLoader settingLoaderMock = mock(SettingLoader.class); + PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB"); + String key = randomAlphanumeric(3); + doThrow(toBeThrown).when(settingLoaderMock).load(key); + underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock); + + assertThat(underTest.get(key)).isEmpty(); + } + + @Test + public void get_returns_empty_if_DB_error_on_first_call_ever_in_thread_cache() { + SettingLoader settingLoaderMock = mock(SettingLoader.class); + PersistenceException toBeThrown = new PersistenceException("Faking an error connecting to DB"); + String key = randomAlphanumeric(3); + doThrow(toBeThrown).when(settingLoaderMock).load(key); + underTest = new ThreadLocalSettings(new PropertyDefinitions(), new Properties(), settingLoaderMock); + underTest.load(); + + assertThat(underTest.get(key)).isEmpty(); + } + private void insertPropertyIntoDb(String key, @Nullable String value) { dbSettingLoader.put(key, value); } -- 2.39.5