Fully delete already-expired accounts
This commit is contained in:
		
							parent
							
								
									a4ca1ef1a8
								
							
						
					
					
						commit
						cf89e2215c
					
				|  | @ -4,33 +4,29 @@ | |||
|  */ | ||||
| package org.whispersystems.textsecuregcm.storage; | ||||
| 
 | ||||
| import com.codahale.metrics.Histogram; | ||||
| import com.codahale.metrics.Meter; | ||||
| import com.codahale.metrics.MetricRegistry; | ||||
| import com.codahale.metrics.SharedMetricRegistries; | ||||
| import com.google.common.annotations.VisibleForTesting; | ||||
| import org.slf4j.Logger; | ||||
| import org.slf4j.LoggerFactory; | ||||
| import org.whispersystems.textsecuregcm.util.Constants; | ||||
| import org.whispersystems.textsecuregcm.util.Util; | ||||
| 
 | ||||
| import java.util.List; | ||||
| import java.util.Optional; | ||||
| import java.util.UUID; | ||||
| import java.util.concurrent.TimeUnit; | ||||
| import io.micrometer.core.instrument.Metrics; | ||||
| import io.micrometer.core.instrument.Tag; | ||||
| import io.micrometer.core.instrument.Tags; | ||||
| import org.slf4j.Logger; | ||||
| import org.slf4j.LoggerFactory; | ||||
| import org.whispersystems.textsecuregcm.util.Util; | ||||
| 
 | ||||
| import static com.codahale.metrics.MetricRegistry.name; | ||||
| import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; | ||||
| 
 | ||||
| public class AccountCleaner extends AccountDatabaseCrawlerListener { | ||||
| 
 | ||||
|   private static final Logger log = LoggerFactory.getLogger(AccountCleaner.class); | ||||
| 
 | ||||
|   private static final MetricRegistry metricRegistry            = SharedMetricRegistries.getOrCreate(Constants.METRICS_NAME); | ||||
|   private static final Meter          expiredAccountsMeter      = metricRegistry.meter(name(AccountCleaner.class, "expiredAccounts")); | ||||
|   private static final Histogram      deletableAccountHistogram = metricRegistry.histogram(name(AccountCleaner.class, "deletableAccountsPerChunk")); | ||||
|   private static final String DELETED_ACCOUNT_COUNTER_NAME = name(AccountCleaner.class, "deletedAccounts"); | ||||
|   private static final String DELETION_REASON_TAG_NAME = "reason"; | ||||
| 
 | ||||
|   @VisibleForTesting | ||||
|   static final int MAX_ACCOUNT_UPDATES_PER_CHUNK = 40; | ||||
|   static final int MAX_ACCOUNT_DELETIONS_PER_CHUNK = 40; | ||||
| 
 | ||||
|   private final AccountsManager accountsManager; | ||||
| 
 | ||||
|  | @ -48,29 +44,30 @@ public class AccountCleaner extends AccountDatabaseCrawlerListener { | |||
| 
 | ||||
|   @Override | ||||
|   protected void onCrawlChunk(Optional<UUID> fromUuid, List<Account> chunkAccounts) { | ||||
|     int accountUpdateCount    = 0; | ||||
|     int deletableAccountCount = 0; | ||||
|     int accountUpdateCount = 0; | ||||
| 
 | ||||
|     for (Account account : chunkAccounts) { | ||||
|       if (isExpired(account)) { | ||||
|         deletableAccountCount++; | ||||
|       } | ||||
|       if (isExpired(account) || needsExplicitRemoval(account)) { | ||||
|         final Tag deletionReason; | ||||
| 
 | ||||
|       if (needsExplicitRemoval(account)) { | ||||
|         expiredAccountsMeter.mark(); | ||||
|         if (isExpired(account)) { | ||||
|           deletionReason = Tag.of(DELETION_REASON_TAG_NAME, "previouslyExpired"); | ||||
|         } else { | ||||
|           deletionReason = Tag.of(DELETION_REASON_TAG_NAME, "newlyExpired"); | ||||
|         } | ||||
| 
 | ||||
|         if (accountUpdateCount < MAX_ACCOUNT_UPDATES_PER_CHUNK) { | ||||
|         if (accountUpdateCount < MAX_ACCOUNT_DELETIONS_PER_CHUNK) { | ||||
|           try { | ||||
|             accountsManager.delete(account, AccountsManager.DeletionReason.EXPIRED); | ||||
|             accountUpdateCount++; | ||||
| 
 | ||||
|             Metrics.counter(DELETED_ACCOUNT_COUNTER_NAME, Tags.of(deletionReason)).increment(); | ||||
|           } catch (final Exception e) { | ||||
|             log.warn("Failed to delete account {}", account.getUuid(), e); | ||||
|           } | ||||
|         } | ||||
|       } | ||||
|     } | ||||
| 
 | ||||
|     deletableAccountHistogram.update(deletableAccountCount); | ||||
|   } | ||||
| 
 | ||||
|   private boolean needsExplicitRemoval(Account account) { | ||||
|  |  | |||
|  | @ -21,11 +21,7 @@ import java.util.UUID; | |||
| import java.util.concurrent.TimeUnit; | ||||
| import org.junit.jupiter.api.Test; | ||||
| import org.junit.jupiter.api.BeforeEach; | ||||
| import org.whispersystems.textsecuregcm.storage.Account; | ||||
| import org.whispersystems.textsecuregcm.storage.AccountCleaner; | ||||
| import org.whispersystems.textsecuregcm.storage.AccountDatabaseCrawlerRestartException; | ||||
| import org.whispersystems.textsecuregcm.storage.AccountsManager; | ||||
| import org.whispersystems.textsecuregcm.storage.Device; | ||||
| import org.whispersystems.textsecuregcm.storage.AccountsManager.DeletionReason; | ||||
| 
 | ||||
| class AccountCleanerTest { | ||||
| 
 | ||||
|  | @ -77,8 +73,8 @@ class AccountCleanerTest { | |||
|     accountCleaner.timeAndProcessCrawlChunk(Optional.empty(), Arrays.asList(deletedDisabledAccount, undeletedDisabledAccount, undeletedEnabledAccount)); | ||||
|     accountCleaner.onCrawlEnd(Optional.empty()); | ||||
| 
 | ||||
|     verify(accountsManager, never()).delete(eq(deletedDisabledAccount), any()); | ||||
|     verify(accountsManager).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED); | ||||
|     verify(accountsManager).delete(deletedDisabledAccount, DeletionReason.EXPIRED); | ||||
|     verify(accountsManager).delete(undeletedDisabledAccount, DeletionReason.EXPIRED); | ||||
|     verify(accountsManager, never()).delete(eq(undeletedEnabledAccount), any()); | ||||
| 
 | ||||
|     verifyNoMoreInteractions(accountsManager); | ||||
|  | @ -89,7 +85,7 @@ class AccountCleanerTest { | |||
|     List<Account> accounts = new LinkedList<>(); | ||||
|     accounts.add(undeletedEnabledAccount); | ||||
| 
 | ||||
|     int activeExpiredAccountCount = AccountCleaner.MAX_ACCOUNT_UPDATES_PER_CHUNK + 1; | ||||
|     int activeExpiredAccountCount = AccountCleaner.MAX_ACCOUNT_DELETIONS_PER_CHUNK + 1; | ||||
|     for (int addedAccountCount = 0; addedAccountCount < activeExpiredAccountCount; addedAccountCount++) { | ||||
|       accounts.add(undeletedDisabledAccount); | ||||
|     } | ||||
|  | @ -101,7 +97,7 @@ class AccountCleanerTest { | |||
|     accountCleaner.timeAndProcessCrawlChunk(Optional.empty(), accounts); | ||||
|     accountCleaner.onCrawlEnd(Optional.empty()); | ||||
| 
 | ||||
|     verify(accountsManager, times(AccountCleaner.MAX_ACCOUNT_UPDATES_PER_CHUNK)).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED); | ||||
|     verify(accountsManager, times(AccountCleaner.MAX_ACCOUNT_DELETIONS_PER_CHUNK)).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED); | ||||
|     verifyNoMoreInteractions(accountsManager); | ||||
|   } | ||||
| 
 | ||||
|  |  | |||
		Loading…
	
		Reference in New Issue
	
	 Jon Chambers
						Jon Chambers