Skip to content

Commit

Permalink
Comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@justinlin-linkedin
justinlin-linkedin committed Aug 29, 2019
1 parent 004e7bc commit 8bdb461
Show file tree
Hide file tree
Showing 9 changed files with 218 additions and 191 deletions.
10 changes: 5 additions & 5 deletions ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,20 @@ abstract class AccountMetadataStore {
private static final Logger logger = LoggerFactory.getLogger(AccountMetadataStore.class);

protected final AccountServiceMetrics accountServiceMetrics;
protected final LocalBackup backup;
protected final BackupFileManager backupFileManager;
protected final String znRecordPath;
private final HelixPropertyStore<ZNRecord> helixStore;

/** Create a new {@link AccountMetadataStore} instance for the subclasses.
* @param accountServiceMetrics The {@link AccountServiceMetrics}
* @param backup The {@link LocalBackup} to manage the backup files.
* @param backupFileManager The {@link BackupFileManager} to manage the backup files.
* @param helixStore The {@link HelixPropertyStore} to retrieve and update the {@link ZNRecord}.
* @param znRecordPath The {@link ZNRecord} path.
*/
AccountMetadataStore(AccountServiceMetrics accountServiceMetrics, LocalBackup backup,
AccountMetadataStore(AccountServiceMetrics accountServiceMetrics, BackupFileManager backupFileManager,
HelixPropertyStore<ZNRecord> helixStore, String znRecordPath) {
this.accountServiceMetrics = accountServiceMetrics;
this.backup = backup;
this.backupFileManager = backupFileManager;
this.helixStore = helixStore;
this.znRecordPath = znRecordPath;
}
Expand Down Expand Up @@ -91,7 +91,7 @@ Map<String, String> fetchAccountMetadata() {
return null;
}
Map<String, String> newAccountMap = fetchAccountMetadataFromZNRecord(znRecord);
backup.persistState(newAccountMap, znRecord);
backupFileManager.persistState(newAccountMap, znRecord);
return newAccountMap;
}

Expand Down
268 changes: 147 additions & 121 deletions ...com/github/ambry/account/LocalBackup.java → ...thub/ambry/account/BackupFileManager.java
View file

Large diffs are not rendered by default.

24 changes: 12 additions & 12 deletions ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public class HelixAccountService implements AccountService {
static final String ACCOUNT_METADATA_CHANGE_TOPIC = "account_metadata_change_topic";
static final String FULL_ACCOUNT_METADATA_CHANGE_MESSAGE = "full_account_metadata_change";

private final LocalBackup backup;
private final BackupFileManager backupFileManager;
private final AccountServiceMetrics accountServiceMetrics;
private final HelixPropertyStore<ZNRecord> helixStore;
private final Notifier<String> notifier;
Expand Down Expand Up @@ -125,16 +125,16 @@ public class HelixAccountService implements AccountService {
this.scheduler = scheduler;
this.config = config;
this.accountInfoMapRef = new AtomicReference<>(new AccountInfoMap(accountServiceMetrics));
this.backup = new LocalBackup(this.accountServiceMetrics, config);
this.backupFileManager = new BackupFileManager(this.accountServiceMetrics, config);
AccountMetadataStore backFillStore = null;
if (config.useNewZNodePath) {
accountMetadataStore = new RouterStore(this.accountServiceMetrics, backup, helixStore, router, false);
accountMetadataStore = new RouterStore(this.accountServiceMetrics, backupFileManager, helixStore, router, false);
// postpone initializeFetchAndSchedule to setupRouter function.
} else {
accountMetadataStore = new LegacyMetadataStore(this.accountServiceMetrics, backup, helixStore);
accountMetadataStore = new LegacyMetadataStore(this.accountServiceMetrics, backupFileManager, helixStore);
initialFetchAndSchedule();
if (config.backFillAccountsToNewZNode) {
backFillStore = new RouterStore(this.accountServiceMetrics, backup, helixStore, router, true);
backFillStore = new RouterStore(this.accountServiceMetrics, backupFileManager, helixStore, router, true);
}
}
this.backFillStore = backFillStore;
Expand Down Expand Up @@ -198,9 +198,9 @@ private void initialFetchAndSchedule() {
// is no account metadata for the time being. Theoretically local storage shouldn't have any backup files. So
// backup.getLatestState should return null. And in case we have a very old backup file just mentioned above, a threshold
// would solve the problem.
if (accountInfoMapRef.get().isEmpty() && config.enableServeFromBackup && !backup.isEmpty()) {
long aMonthAgo = System.currentTimeMillis() / 1000 - 30 * 24 * 60 * 60;
Map<String, String> accountMap = backup.getLatestState(aMonthAgo);
if (accountInfoMapRef.get().isEmpty() && config.enableServeFromBackup && !backupFileManager.isEmpty()) {
long aMonthAgo = System.currentTimeMillis() / 1000 - TimeUnit.DAYS.toSeconds(30);
Map<String, String> accountMap = backupFileManager.getLatestState(aMonthAgo);
if (accountMap != null) {
AccountInfoMap newAccountInfoMap = new AccountInfoMap(accountServiceMetrics, accountMap);
AccountInfoMap oldAccountInfoMap = accountInfoMapRef.getAndSet(newAccountInfoMap);
Expand Down Expand Up @@ -441,11 +441,11 @@ AccountServiceMetrics getAccountServiceMetrics() {
}

/**
* Return {@link LocalBackup}.
* @return {@link LocalBackup}
* Return {@link BackupFileManager}.
* @return {@link BackupFileManager}
*/
LocalBackup getBackup() {
return backup;
BackupFileManager getBackupFileManager() {
return backupFileManager;
}
}

Expand Down
6 changes: 3 additions & 3 deletions ambry-account/src/main/java/com/github/ambry/account/LegacyMetadataStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ class LegacyMetadataStore extends AccountMetadataStore {
/**
* Constructor to create a {@link LegacyMetadataStore}.
* @param accountServiceMetrics The metrics set to update metrics.
* @param backup The {@link LocalBackup} instance to manage backup files.
* @param backupFileManager The {@link BackupFileManager} instance to manage backup files.
* @param helixStore The {@link HelixPropertyStore} to fetch and update data.
*/
LegacyMetadataStore(AccountServiceMetrics accountServiceMetrics, LocalBackup backup,
LegacyMetadataStore(AccountServiceMetrics accountServiceMetrics, BackupFileManager backupFileManager,
HelixPropertyStore<ZNRecord> helixStore) {
super(accountServiceMetrics, backup, helixStore, FULL_ACCOUNT_METADATA_PATH);
super(accountServiceMetrics, backupFileManager, helixStore, FULL_ACCOUNT_METADATA_PATH);
}

@Override
Expand Down
9 changes: 4 additions & 5 deletions ambry-account/src/main/java/com/github/ambry/account/RouterStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,14 @@ class RouterStore extends AccountMetadataStore {
/**
* Constructor to create the RouterStore.
* @param accountServiceMetrics The metrics set to update metrics.
* @param backup The {@link LocalBackup} instance to manage backup files.
* @param backupFileManager The {@link BackupFileManager} instance to manage backup files.
* @param helixStore The {@link HelixPropertyStore} to fetch and update data.
* @param router The {@link Router} instance to retrieve and put blobs.
* @param forBackFill True if this {@link RouterStore} is created for backfill accounts to new zookeeper node.
*/
RouterStore(AccountServiceMetrics accountServiceMetrics, LocalBackup backup, HelixPropertyStore<ZNRecord> helixStore,
AtomicReference<Router> router, boolean forBackFill) {
super(accountServiceMetrics, backup, helixStore, ACCOUNT_METADATA_BLOB_IDS_PATH);
RouterStore(AccountServiceMetrics accountServiceMetrics, BackupFileManager backupFileManager,
HelixPropertyStore<ZNRecord> helixStore, AtomicReference<Router> router, boolean forBackFill) {
super(accountServiceMetrics, backupFileManager, helixStore, ACCOUNT_METADATA_BLOB_IDS_PATH);
this.router = router;
this.forBackFill = forBackFill;
}
Expand Down Expand Up @@ -247,7 +247,6 @@ public ZNRecord update(ZNRecord znRecord) {
logger.error(errorMessage, e);
throw new IllegalStateException(errorMessage, e);
}
backup.maybePersistOldState(backupPrefixAndPath, accountMap);

// if there is any conflict with the existing record, fail the update. Exception thrown in this updater will
// be caught by Helix and helixStore#update will return false.
Expand Down
64 changes: 34 additions & 30 deletions ...github/ambry/account/LocalBackupTest.java → ...ry/account/BackupFileInfoManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@


/**
* Unit test for {@link LocalBackup}
* Unit test for {@link BackupFileManager}
*/
public class LocalBackupTest {
public class BackupFileInfoManagerTest {
private final AccountServiceMetrics accountServiceMetrics = new AccountServiceMetrics(new MetricRegistry());
private final Properties helixConfigProps = new Properties();
private static final int ZK_CLIENT_CONNECTION_TIMEOUT_MS = 20000;
Expand All @@ -55,10 +55,10 @@ public class LocalBackupTest {
private Account refAccount;

/**
* Constructor to create a LocalBackupTest
* Constructor to create a BackupFileInfoManagerTest
* @throws IOException if I/O error occurs
*/
public LocalBackupTest() throws IOException {
public BackupFileInfoManagerTest() throws IOException {
accountBackupDir = Paths.get(TestUtils.getTempDir("account-backup")).toAbsolutePath();
helixConfigProps.setProperty(HelixAccountServiceConfig.BACKUP_DIRECTORY_KEY, accountBackupDir.toString());
helixConfigProps.setProperty(HelixAccountServiceConfig.MAX_BACKUP_FILE_COUNT, String.valueOf(maxBackupFile));
Expand All @@ -83,15 +83,15 @@ public void cleanUp() throws IOException {
}

/**
* Test disable backup. Every method from {@link LocalBackup} should return null or have no effect.
* Test disable backup. Every method from {@link BackupFileManager} should return null or have no effect.
* @throws IOException if I/O error occurs
*/
@Test
public void testDisableBackupDir() throws IOException {
helixConfigProps.remove(HelixAccountServiceConfig.BACKUP_DIRECTORY_KEY);
VerifiableProperties vHelixConfigProps = new VerifiableProperties(helixConfigProps);
HelixAccountServiceConfig config = new HelixAccountServiceConfig(vHelixConfigProps);
LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);

// getLatestState should return null
assertNull("Disabled backup shouldn't have any state", backup.getLatestState(0));
Expand Down Expand Up @@ -122,7 +122,7 @@ public void testGetLatestState_BackupFilesAllWithVersion() throws IOException {
Map<String, String> expected = new HashMap<>();
expected.put(String.valueOf(refAccount.getId()), refAccount.toJson(true).toString());

LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
Map<String, String> obtained = backup.getLatestState(0);
assertTwoStringMapsEqual(expected, obtained);
assertFalse("There are backup files", backup.isEmpty());
Expand All @@ -144,7 +144,7 @@ public void testGetLatestState_BackupFilesAllWithoutVersion() throws IOException
String[] filenames = createBackupFilesWithoutVersion(accountBackupDir, count, baseModifiedTime, interval, false);
saveAccountsToFile(Collections.singleton(refAccount), accountBackupDir.resolve(filenames[filenames.length - 1]));

LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
Map<String, String> obtained = backup.getLatestState(0);
assertNull("No state should be loaded from backup file from old format", obtained);
assertFalse("Backup should not be empty since there are files from old format", backup.isEmpty());
Expand All @@ -171,15 +171,15 @@ public void testGetLatestState_MixedBackupFiles() throws IOException {
Map<String, String> expected = new HashMap<>();
expected.put(String.valueOf(refAccount.getId()), refAccount.toJson(true).toString());

LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
Map<String, String> obtained = backup.getLatestState(0);
assertTwoStringMapsEqual(expected, obtained);
assertFalse("There are backup files", backup.isEmpty());
assertEquals(backup.size(), endVersion - startVersion + 1 + backupFileNoVersionCount);
}

/**
* Test if LocalBackup correctly cleans up the backup files, when there are more than {@link #maxBackupFile} backup
* Test if BackupFileManager correctly cleans up the backup files, when there are more than {@link #maxBackupFile} backup
* files with version number.
* @throws IOException if I/O error occurs
*/
Expand All @@ -201,20 +201,20 @@ public void testCleanup_EnoughBackupFilesWithVersion() throws IOException {
createBackupFilesWithoutVersion(accountBackupDir, 10, baseModifiedTime, interval, false);
createBackupFilesWithoutVersion(accountBackupDir, 10, baseModifiedTime, interval, true);

LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
assertEquals("Number of backup files mismatch", backup.size(), maxBackupFile);

File[] remainingFiles = accountBackupDir.toFile().listFiles();
assertEquals("Number of backup files in storage mismatch", remainingFiles.length, maxBackupFile);
for (File file : remainingFiles) {
// All the files should
assertTrue("Filename " + file.getName() + " should follow version pattern ",
LocalBackup.versionFilenamePattern.matcher(file.getName()).find());
BackupFileManager.versionFilenamePattern.matcher(file.getName()).find());
}
}

/**
* Test if LocalBackup correctly cleans up the backup files, when there are less than {@link #maxBackupFile} backup
* Test if BackupFileManager correctly cleans up the backup files, when there are less than {@link #maxBackupFile} backup
* files with version number.
* @throws IOException if I/O error occurs
*/
Expand All @@ -236,15 +236,15 @@ public void testCleanup_NotEnoughBackupFilesWithVersion() throws IOException {
baseModifiedTime + 2 * backupFileNoVersionCount * interval, interval, false);
saveAccountsToFile(Collections.singleton(refAccount), accountBackupDir.resolve(filenames[filenames.length - 1]));

LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
assertEquals("Number of backup files mismatch", backup.size(), maxBackupFile);

File[] remainingFiles = accountBackupDir.toFile().listFiles();
assertEquals("Number of backup files in storage mismatch", remainingFiles.length, maxBackupFile);
int versionFileCount = 0;
int nonVersionFileCount = 0;
for (File file : remainingFiles) {
if (LocalBackup.versionFilenamePattern.matcher(file.getName()).find()) {
if (BackupFileManager.versionFilenamePattern.matcher(file.getName()).find()) {
versionFileCount++;
} else {
nonVersionFileCount++;
Expand All @@ -256,12 +256,12 @@ public void testCleanup_NotEnoughBackupFilesWithVersion() throws IOException {
}

/**
* Test {@link LocalBackup#persistState(Map, ZNRecord)} and then recover {@link LocalBackup} from the same backup directory.
* Test {@link BackupFileManager#persistState(Map, ZNRecord)} and then recover {@link BackupFileManager} from the same backup directory.
* @throws IOException if I/O error occurs
*/
@Test
public void testPersistStateAndRecover() throws IOException {
LocalBackup backup = new LocalBackup(accountServiceMetrics, config);
BackupFileManager backup = new BackupFileManager(accountServiceMetrics, config);
final int numberAccounts = maxBackupFile * 2;
final Map<String, String> accounts = new HashMap<>(numberAccounts);
final ZNRecord record = new ZNRecord(String.valueOf(System.currentTimeMillis()));
Expand All @@ -283,8 +283,8 @@ public void testPersistStateAndRecover() throws IOException {
File[] remaingingFiles = accountBackupDir.toFile().listFiles();
assertEquals("Remaining backup mismatch", maxBackupFile, remaingingFiles.length);

// Recover LocalBackup from the same backup dir
backup = new LocalBackup(accountServiceMetrics, config);
// Recover BackupFileManager from the same backup dir
backup = new BackupFileManager(accountServiceMetrics, config);
}
}

Expand All @@ -296,14 +296,14 @@ public void testPersistStateAndRecover() throws IOException {
public void testSerializationAndDeserialization() throws JSONException {
// Since the account metadata is stored in an map from string to string, so here we only test map<string, string>
Map<String, String> state = new HashMap<>();
ByteBuffer buffer = LocalBackup.serializeState(state);
Map<String, String> deserializedState = LocalBackup.deserializeState(buffer.array());
ByteBuffer buffer = BackupFileManager.serializeState(state);
Map<String, String> deserializedState = BackupFileManager.deserializeState(buffer.array());
assertTwoStringMapsEqual(state, deserializedState);

// Put a real account's json string
state.put(String.valueOf(refAccount.getId()), refAccount.toJson(true).toString());
buffer = LocalBackup.serializeState(state);
deserializedState = LocalBackup.deserializeState(buffer.array());
buffer = BackupFileManager.serializeState(state);
deserializedState = BackupFileManager.deserializeState(buffer.array());
assertTwoStringMapsEqual(state, deserializedState);
}

Expand Down Expand Up @@ -337,9 +337,9 @@ private String[] createBackupFilesWithVersion(Path backupDir, int startVersion,
ZNRecord record = new ZNRecord(String.valueOf(i));
record.setVersion(i);
record.setModifiedTime(baseModifiedTime + (i - startVersion) * interval);
String filename = LocalBackup.getBackupFilenameFromZNRecord(record);
String filename = BackupFileManager.getBackupFilenameFromZNRecord(record);
if (isTemp) {
filename = filename + LocalBackup.SEP + LocalBackup.TEMP_FILE_SUFFIX;
filename = filename + BackupFileManager.SEP + BackupFileManager.TEMP_FILE_SUFFIX;
}
try {
Files.createFile(backupDir.resolve(filename));
Expand All @@ -365,11 +365,11 @@ private String[] createBackupFilesWithoutVersion(Path backupDir, int count, long
String[] filenames = new String[count];
for (int i = 0; i < count; i++) {
long modifiedTime = baseModifiedTime + i * interval;
String timestamp =
LocalDateTime.ofEpochSecond(modifiedTime, 0, LocalBackup.zoneOffset).format(LocalBackup.TIMESTAMP_FORMATTER);
String filename = timestamp + LocalBackup.SEP + LocalBackup.NEW_STATE_SUFFIX;
String timestamp = LocalDateTime.ofEpochSecond(modifiedTime, 0, BackupFileManager.zoneOffset)
.format(BackupFileManager.TIMESTAMP_FORMATTER);
String filename = timestamp + BackupFileManager.SEP + BackupFileManager.NEW_STATE_SUFFIX;
if (isOld) {
filename = timestamp + LocalBackup.SEP + LocalBackup.OLD_STATE_SUFFIX;
filename = timestamp + BackupFileManager.SEP + BackupFileManager.OLD_STATE_SUFFIX;
}
try {
Files.createFile(backupDir.resolve(filename));
Expand Down Expand Up @@ -424,6 +424,10 @@ private static void saveAccountsToFile(Collection<Account> accounts, Path filePa
} catch (JSONException e) {
fail("Fail to get a json format of accounts");
}
assertTrue("Failed to write state to file", LocalBackup.writeStateToFile(filePath, accountMap));
try {
BackupFileManager.writeStateToFile(filePath, accountMap);
} catch (Exception e) {
fail("Fail to write state to file: " + filePath.toString());
}
}
}
Loading

0 comments on commit 8bdb461

Please sign in to comment.