Allow the account cleaner to act on all accounts in a crawled chunk
This commit is contained in:
		
							parent
							
								
									483fb0968b
								
							
						
					
					
						commit
						2881c0fd7e
					
				| 
						 | 
					@ -25,9 +25,6 @@ public class AccountCleaner extends AccountDatabaseCrawlerListener {
 | 
				
			||||||
  private static final String DELETED_ACCOUNT_COUNTER_NAME = name(AccountCleaner.class, "deletedAccounts");
 | 
					  private static final String DELETED_ACCOUNT_COUNTER_NAME = name(AccountCleaner.class, "deletedAccounts");
 | 
				
			||||||
  private static final String DELETION_REASON_TAG_NAME = "reason";
 | 
					  private static final String DELETION_REASON_TAG_NAME = "reason";
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  @VisibleForTesting
 | 
					 | 
				
			||||||
  static final int MAX_ACCOUNT_DELETIONS_PER_CHUNK = 256;
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
  private final AccountsManager accountsManager;
 | 
					  private final AccountsManager accountsManager;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  public AccountCleaner(AccountsManager accountsManager) {
 | 
					  public AccountCleaner(AccountsManager accountsManager) {
 | 
				
			||||||
| 
						 | 
					@ -44,8 +41,6 @@ public class AccountCleaner extends AccountDatabaseCrawlerListener {
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  @Override
 | 
					  @Override
 | 
				
			||||||
  protected void onCrawlChunk(Optional<UUID> fromUuid, List<Account> chunkAccounts) {
 | 
					  protected void onCrawlChunk(Optional<UUID> fromUuid, List<Account> chunkAccounts) {
 | 
				
			||||||
    int accountUpdateCount = 0;
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
    for (Account account : chunkAccounts) {
 | 
					    for (Account account : chunkAccounts) {
 | 
				
			||||||
      if (isExpired(account) || needsExplicitRemoval(account)) {
 | 
					      if (isExpired(account) || needsExplicitRemoval(account)) {
 | 
				
			||||||
        final Tag deletionReason;
 | 
					        final Tag deletionReason;
 | 
				
			||||||
| 
						 | 
					@ -56,15 +51,11 @@ public class AccountCleaner extends AccountDatabaseCrawlerListener {
 | 
				
			||||||
          deletionReason = Tag.of(DELETION_REASON_TAG_NAME, "previouslyExpired");
 | 
					          deletionReason = Tag.of(DELETION_REASON_TAG_NAME, "previouslyExpired");
 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        if (accountUpdateCount < MAX_ACCOUNT_DELETIONS_PER_CHUNK) {
 | 
					        try {
 | 
				
			||||||
          try {
 | 
					          accountsManager.delete(account, AccountsManager.DeletionReason.EXPIRED);
 | 
				
			||||||
            accountsManager.delete(account, AccountsManager.DeletionReason.EXPIRED);
 | 
					          Metrics.counter(DELETED_ACCOUNT_COUNTER_NAME, Tags.of(deletionReason)).increment();
 | 
				
			||||||
            accountUpdateCount++;
 | 
					        } catch (final Exception e) {
 | 
				
			||||||
 | 
					          log.warn("Failed to delete account {}", account.getUuid(), e);
 | 
				
			||||||
            Metrics.counter(DELETED_ACCOUNT_COUNTER_NAME, Tags.of(deletionReason)).increment();
 | 
					 | 
				
			||||||
          } catch (final Exception e) {
 | 
					 | 
				
			||||||
            log.warn("Failed to delete account {}", account.getUuid(), e);
 | 
					 | 
				
			||||||
          }
 | 
					 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
      }
 | 
					      }
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
| 
						 | 
					
 | 
				
			||||||
| 
						 | 
					@ -8,19 +8,16 @@ import static org.mockito.ArgumentMatchers.any;
 | 
				
			||||||
import static org.mockito.ArgumentMatchers.eq;
 | 
					import static org.mockito.ArgumentMatchers.eq;
 | 
				
			||||||
import static org.mockito.Mockito.mock;
 | 
					import static org.mockito.Mockito.mock;
 | 
				
			||||||
import static org.mockito.Mockito.never;
 | 
					import static org.mockito.Mockito.never;
 | 
				
			||||||
import static org.mockito.Mockito.times;
 | 
					 | 
				
			||||||
import static org.mockito.Mockito.verify;
 | 
					import static org.mockito.Mockito.verify;
 | 
				
			||||||
import static org.mockito.Mockito.verifyNoMoreInteractions;
 | 
					import static org.mockito.Mockito.verifyNoMoreInteractions;
 | 
				
			||||||
import static org.mockito.Mockito.when;
 | 
					import static org.mockito.Mockito.when;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
import java.util.Arrays;
 | 
					import java.util.Arrays;
 | 
				
			||||||
import java.util.LinkedList;
 | 
					 | 
				
			||||||
import java.util.List;
 | 
					 | 
				
			||||||
import java.util.Optional;
 | 
					import java.util.Optional;
 | 
				
			||||||
import java.util.UUID;
 | 
					import java.util.UUID;
 | 
				
			||||||
import java.util.concurrent.TimeUnit;
 | 
					import java.util.concurrent.TimeUnit;
 | 
				
			||||||
import org.junit.jupiter.api.Test;
 | 
					 | 
				
			||||||
import org.junit.jupiter.api.BeforeEach;
 | 
					import org.junit.jupiter.api.BeforeEach;
 | 
				
			||||||
 | 
					import org.junit.jupiter.api.Test;
 | 
				
			||||||
import org.whispersystems.textsecuregcm.storage.AccountsManager.DeletionReason;
 | 
					import org.whispersystems.textsecuregcm.storage.AccountsManager.DeletionReason;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
class AccountCleanerTest {
 | 
					class AccountCleanerTest {
 | 
				
			||||||
| 
						 | 
					@ -80,25 +77,4 @@ class AccountCleanerTest {
 | 
				
			||||||
    verifyNoMoreInteractions(accountsManager);
 | 
					    verifyNoMoreInteractions(accountsManager);
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  @Test
 | 
					 | 
				
			||||||
  void testMaxAccountUpdates() throws AccountDatabaseCrawlerRestartException, InterruptedException {
 | 
					 | 
				
			||||||
    List<Account> accounts = new LinkedList<>();
 | 
					 | 
				
			||||||
    accounts.add(undeletedEnabledAccount);
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
    int activeExpiredAccountCount = AccountCleaner.MAX_ACCOUNT_DELETIONS_PER_CHUNK + 1;
 | 
					 | 
				
			||||||
    for (int addedAccountCount = 0; addedAccountCount < activeExpiredAccountCount; addedAccountCount++) {
 | 
					 | 
				
			||||||
      accounts.add(undeletedDisabledAccount);
 | 
					 | 
				
			||||||
    }
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
    accounts.add(deletedDisabledAccount);
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
    AccountCleaner accountCleaner = new AccountCleaner(accountsManager);
 | 
					 | 
				
			||||||
    accountCleaner.onCrawlStart();
 | 
					 | 
				
			||||||
    accountCleaner.timeAndProcessCrawlChunk(Optional.empty(), accounts);
 | 
					 | 
				
			||||||
    accountCleaner.onCrawlEnd(Optional.empty());
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
    verify(accountsManager, times(AccountCleaner.MAX_ACCOUNT_DELETIONS_PER_CHUNK)).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED);
 | 
					 | 
				
			||||||
    verifyNoMoreInteractions(accountsManager);
 | 
					 | 
				
			||||||
  }
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
| 
						 | 
					
 | 
				
			||||||
		Loading…
	
		Reference in New Issue