From 7f8f2641f61d69b00d082382cb6878dd9e10a9ab Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Thu, 16 Jul 2020 13:55:38 -0400 Subject: [PATCH] Simplify registration lock counting by avoiding inactive accounts. --- .../RegistrationLockVersionCounter.java | 44 ++++++--------- .../RegistrationLockVersionCounterTest.java | 53 ++++++------------- 2 files changed, 32 insertions(+), 65 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounter.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounter.java index 425ff0bd0..ff68c3a8b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounter.java @@ -26,10 +26,8 @@ public class RegistrationLockVersionCounter extends AccountDatabaseCrawlerListen private final MetricsFactory metricsFactory; static final String REGLOCK_COUNT_KEY = "ReglockVersionCounter::reglockCount"; - static final String NO_REGLOCK_KEY = "none"; - static final String PIN_ONLY_KEY = "pinOnly"; - static final String REGLOCK_ONLY_KEY = "reglockOnly"; - static final String BOTH_KEY = "both"; + static final String PIN_KEY = "pin"; + static final String REGLOCK_KEY = "reglock"; public RegistrationLockVersionCounter(final FaultTolerantRedisCluster redisCluster, final MetricsFactory metricsFactory) { this.redisCluster = redisCluster; @@ -38,52 +36,42 @@ public class RegistrationLockVersionCounter extends AccountDatabaseCrawlerListen @Override public void onCrawlStart() { - redisCluster.useWriteCluster(connection -> connection.sync().hset(REGLOCK_COUNT_KEY, Map.of( - NO_REGLOCK_KEY, "0", - PIN_ONLY_KEY, "0", - REGLOCK_ONLY_KEY, "0", - BOTH_KEY, "0"))); + redisCluster.useWriteCluster(connection -> connection.sync().hset(REGLOCK_COUNT_KEY, Map.of(PIN_KEY, "0", REGLOCK_KEY, "0"))); } @Override protected void onCrawlChunk(final Optional fromUuid, final List chunkAccounts) { - int noReglockCount = 0; - int pinOnlyCount = 0; - int reglockOnlyCount = 0; - int bothCount = 0; + int pinCount = 0; + int reglockCount = 0; for (final Account account : chunkAccounts) { final StoredRegistrationLock storedRegistrationLock = account.getRegistrationLock(); - if (storedRegistrationLock.hasDeprecatedPin() && storedRegistrationLock.needsFailureCredentials()) { - bothCount++; - } else if (storedRegistrationLock.hasDeprecatedPin()) { - pinOnlyCount++; - } else if (storedRegistrationLock.needsFailureCredentials()) { - reglockOnlyCount++; - } else { - noReglockCount++; + if (storedRegistrationLock.requiresClientRegistrationLock()) { + if (storedRegistrationLock.hasDeprecatedPin()) { + pinCount++; + } else { + reglockCount++; + } } } - incrementReglockCounts(noReglockCount, pinOnlyCount, reglockOnlyCount, bothCount); + incrementReglockCounts(pinCount, reglockCount); } - private void incrementReglockCounts(final int noReglockCount, final int pinOnlyCount, final int reglockOnlyCount, final int bothCount) { + private void incrementReglockCounts(final int pinCount, final int reglockCount) { redisCluster.useWriteCluster(connection -> { final RedisAdvancedClusterCommands commands = connection.sync(); - commands.hincrby(REGLOCK_COUNT_KEY, NO_REGLOCK_KEY, noReglockCount); - commands.hincrby(REGLOCK_COUNT_KEY, PIN_ONLY_KEY, pinOnlyCount); - commands.hincrby(REGLOCK_COUNT_KEY, REGLOCK_ONLY_KEY, reglockOnlyCount); - commands.hincrby(REGLOCK_COUNT_KEY, BOTH_KEY, bothCount); + commands.hincrby(REGLOCK_COUNT_KEY, PIN_KEY, pinCount); + commands.hincrby(REGLOCK_COUNT_KEY, REGLOCK_KEY, reglockCount); }); } @Override public void onCrawlEnd(final Optional fromUuid) { final Map countsByReglockType = - redisCluster.withReadCluster(connection -> connection.sync().hmget(REGLOCK_COUNT_KEY, NO_REGLOCK_KEY, PIN_ONLY_KEY, REGLOCK_ONLY_KEY, BOTH_KEY)) + redisCluster.withReadCluster(connection -> connection.sync().hmget(REGLOCK_COUNT_KEY, PIN_KEY, REGLOCK_KEY)) .stream() .collect(Collectors.toMap(KeyValue::getKey, keyValue -> keyValue.hasValue() ? keyValue.map(Integer::parseInt).getValue() : 0)); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounterTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounterTest.java index 8cd2fcdb5..efc99a39a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounterTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationLockVersionCounterTest.java @@ -52,63 +52,46 @@ public class RegistrationLockVersionCounterTest { registrationLockVersionCounter.onCrawlChunk(Optional.empty(), List.of(account)); - verifyCount(1, 0, 0, 0); + verifyCount(0, 0); } @Test - public void testOnCrawlChunkPinOnly() { + public void testOnCrawlChunkPin() { final Account account = mock(Account.class); final StoredRegistrationLock registrationLock = mock(StoredRegistrationLock.class); when(account.getRegistrationLock()).thenReturn(registrationLock); + when(registrationLock.requiresClientRegistrationLock()).thenReturn(true); when(registrationLock.hasDeprecatedPin()).thenReturn(true); - when(registrationLock.needsFailureCredentials()).thenReturn(false); registrationLockVersionCounter.onCrawlChunk(Optional.empty(), List.of(account)); - verifyCount(0, 1, 0, 0); + verifyCount(1, 0); } @Test - public void testOnCrawlChunkReglockOnly() { + public void testOnCrawlChunkReglock() { final Account account = mock(Account.class); final StoredRegistrationLock registrationLock = mock(StoredRegistrationLock.class); when(account.getRegistrationLock()).thenReturn(registrationLock); + when(registrationLock.requiresClientRegistrationLock()).thenReturn(true); when(registrationLock.hasDeprecatedPin()).thenReturn(false); - when(registrationLock.needsFailureCredentials()).thenReturn(true); registrationLockVersionCounter.onCrawlChunk(Optional.empty(), List.of(account)); - verifyCount(0, 0, 1, 0); + verifyCount(0, 1); } - @Test - public void testOnCrawlChunkBoth() { - final Account account = mock(Account.class); - final StoredRegistrationLock registrationLock = mock(StoredRegistrationLock.class); - - when(account.getRegistrationLock()).thenReturn(registrationLock); - when(registrationLock.hasDeprecatedPin()).thenReturn(true); - when(registrationLock.needsFailureCredentials()).thenReturn(true); - - registrationLockVersionCounter.onCrawlChunk(Optional.empty(), List.of(account)); - - verifyCount(0, 0, 0, 1); - } - - private void verifyCount(final int noReglock, final int pinOnly, final int reglockOnly, final int both) { - verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.NO_REGLOCK_KEY, noReglock); - verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.PIN_ONLY_KEY, pinOnly); - verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.REGLOCK_ONLY_KEY, reglockOnly); - verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.BOTH_KEY, both); + private void verifyCount(final int pinCount, final int reglockCount) { + verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.PIN_KEY, pinCount); + verify(redisCommands).hincrby(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY, RegistrationLockVersionCounter.REGLOCK_KEY, reglockCount); } @Test public void testOnCrawlEnd() { - final int noReglockCount = 21; - final int pinOnlyCount = 7; - final int reglockOnlyCount = 83; + final int pinCount = 7; + final int reglockCount = 83; final ReporterFactory reporterFactory = mock(ReporterFactory.class); final ScheduledReporter reporter = mock(ScheduledReporter.class); @@ -119,10 +102,8 @@ public class RegistrationLockVersionCounterTest { when(reporterFactory.build(any())).thenReturn(reporter); when(redisCommands.hmget(eq(RegistrationLockVersionCounter.REGLOCK_COUNT_KEY), any())).thenReturn(List.of( - KeyValue.just(RegistrationLockVersionCounter.NO_REGLOCK_KEY, String.valueOf(noReglockCount)), - KeyValue.just(RegistrationLockVersionCounter.PIN_ONLY_KEY, String.valueOf(pinOnlyCount)), - KeyValue.just(RegistrationLockVersionCounter.REGLOCK_ONLY_KEY, String.valueOf(reglockOnlyCount)), - KeyValue.empty(RegistrationLockVersionCounter.BOTH_KEY))); + KeyValue.just(RegistrationLockVersionCounter.PIN_KEY, String.valueOf(pinCount)), + KeyValue.just(RegistrationLockVersionCounter.REGLOCK_KEY, String.valueOf(reglockCount)))); registrationLockVersionCounter.onCrawlEnd(Optional.empty()); @@ -130,9 +111,7 @@ public class RegistrationLockVersionCounterTest { verify(reporter).report(); @SuppressWarnings("rawtypes") final Map gauges = registryCaptor.getValue().getGauges(); - assertEquals(noReglockCount, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.NO_REGLOCK_KEY)).getValue()); - assertEquals(pinOnlyCount, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.PIN_ONLY_KEY)).getValue()); - assertEquals(reglockOnlyCount, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.REGLOCK_ONLY_KEY)).getValue()); - assertEquals(0, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.BOTH_KEY)).getValue()); + assertEquals(pinCount, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.PIN_KEY)).getValue()); + assertEquals(reglockCount, gauges.get(name(RegistrationLockVersionCounter.class, RegistrationLockVersionCounter.REGLOCK_KEY)).getValue()); } }