Basic implementation of MySqlAccountService #3
Basic implementation of MySqlAccountService #3Arun-LinkedIn wants to merge 3 commits intolightningrob:mysql-accountsfrom
Conversation
…ities. a) Fetch all accounts and container metadata from DB and initialize cache on boot-up, b) Implement AccountService interface update APIs to write changes to mysql database and getter APIs to read changes from cache.
lightningrob
left a comment
lightningrob
left a comment
There was a problem hiding this comment.
Good start. A few comments on the code. Tests still needed.
ambry-account/src/main/java/com/github/ambry/account/mysql/AccountDao.java
Show resolved
Hide resolved
| getByAccountSql = | ||
| String.format("select %s, %s, %s from %s where %s = ?", ACCOUNT_ID, CONTAINER_INFO, LAST_MODIFIED_TIME, | ||
| CONTAINER_TABLE, ACCOUNT_ID); | ||
| updateSql = String.format("update %s set %s = ?, %s = 1, %s = now() where %s = ? AND %s = ? ", CONTAINER_TABLE, |
There was a problem hiding this comment.
You don't want to set version = 1 on update.
There was a problem hiding this comment.
Sure, true. Added a todo to change it after adding version field in Container class. I think that we might need to add version field on both ambry and nuage-ambry at same time to avoid serialization/deserialization issues.
ambry-account/src/main/java/com/github/ambry/account/mysql/AccountDao.java
Outdated
Show resolved
Hide resolved
| return snapshotVersion; | ||
| } | ||
|
|
||
| public void setStatus(AccountStatus status) { |
There was a problem hiding this comment.
Why were these two setters added?
There was a problem hiding this comment.
I had added them to help with modification of existing Accounts in cache (were being called from AccountInfoMap::updateAccounts()) . But, based on the java docs, it looks like Account and Container classes are supposed to be treated as immutable objects. Changed code to use AccountBuilder to re-build new Account objects while updating accounts.
| try { | ||
| createMySqlAccountStore(); | ||
| } catch (SQLException e) { | ||
| logger.error("MySQL account store creation failed", e); |
There was a problem hiding this comment.
This is okay for now. We will need to distinguish between transient connection error and something like credential issue which should fail startup.
| * Sets the last modified time of accounts and containers in this in-memory cache | ||
| * @param lastModifiedTime time when the accounts and containers were last updated | ||
| */ | ||
| void setLastModifiedTime(long lastModifiedTime) { |
There was a problem hiding this comment.
This will be called from MySqlAccountService::fetchAndUpdateCache(). Want to set it to max LMT value found in fetched accounts and containers.
ambry-account/src/main/java/com/github/ambry/account/MySqlAccountStore.java
Outdated
Show resolved
Hide resolved
|
|
||
| // Fetch all added/modified containers from MySql database since LMT | ||
| List<Container> containers = mySqlAccountStore.getNewContainers(lastModifiedTime); | ||
| rwLock.writeLock().lock(); |
There was a problem hiding this comment.
I would recommend holding the lock for both account and container update, to make sure consumers see a consistent view of the cache.
There was a problem hiding this comment.
Sure, yeah true. Made changes to hold the lock for both account and container updates.
ambry-account/src/main/java/com/github/ambry/account/MySqlAccountService.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/MySqlAccountService.java
Outdated
Show resolved
Hide resolved
1. Address Rob's comments 2. Scheduler thread to sync changes from DB 3. Add MySqlAccountServiceFactory and MySqlAccountServiceConfig
2 participants
Changes implemented:
a) Fetch all accounts and container metadata from DB and initialize cache on boot-up,
b) Implement AccountService interface update APIs to write changes to mysql database and getter APIs to read changes from cache.