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.
| 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.
| 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.
|
|
||
| // 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.
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.