Skip to content

Commit

Permalink
Introduce 2 new ehcaches to take care of immutable account data and B…
Browse files Browse the repository at this point in the history 
…CD (updated only once).

The code also refactors the account API to make sure any path (recordId, id, externalKey) whether from public or private API
will end up setting the cache.
  • Loading branch information
@sbrossie
sbrossie committed Oct 2, 2015
1 parent 5bec17d commit 8637993
Show file tree
Hide file tree
Showing 23 changed files with 427 additions and 106 deletions.
102 changes: 70 additions & 32 deletions account/src/main/java/org/killbill/billing/account/api/svcs/DefaultAccountInternalApi.java
View file
Expand Up @@ -22,52 +22,72 @@
import javax.inject.Inject; import javax.inject.Inject;


import org.killbill.billing.ErrorCode; import org.killbill.billing.ErrorCode;
import org.killbill.billing.ObjectType;
import org.killbill.billing.account.api.Account; import org.killbill.billing.account.api.Account;
import org.killbill.billing.account.api.AccountApiException; import org.killbill.billing.account.api.AccountApiException;
import org.killbill.billing.account.api.AccountEmail; import org.killbill.billing.account.api.AccountEmail;
import org.killbill.billing.account.api.AccountInternalApi; import org.killbill.billing.account.api.AccountInternalApi;
import org.killbill.billing.account.api.DefaultAccount;
import org.killbill.billing.account.api.DefaultAccountEmail; import org.killbill.billing.account.api.DefaultAccountEmail;
import org.killbill.billing.account.api.DefaultImmutableAccountData; import org.killbill.billing.account.api.DefaultImmutableAccountData;
import org.killbill.billing.account.api.DefaultMutableAccountData;
import org.killbill.billing.account.api.ImmutableAccountData; import org.killbill.billing.account.api.ImmutableAccountData;
import org.killbill.billing.account.api.MutableAccountData; import org.killbill.billing.account.api.MutableAccountData;
import org.killbill.billing.account.api.user.DefaultAccountApiBase;
import org.killbill.billing.account.dao.AccountDao; import org.killbill.billing.account.dao.AccountDao;
import org.killbill.billing.account.dao.AccountEmailModelDao; import org.killbill.billing.account.dao.AccountEmailModelDao;
import org.killbill.billing.account.dao.AccountModelDao; import org.killbill.billing.account.dao.AccountModelDao;
import org.killbill.billing.callcontext.InternalCallContext; import org.killbill.billing.callcontext.InternalCallContext;
import org.killbill.billing.callcontext.InternalTenantContext; import org.killbill.billing.callcontext.InternalTenantContext;
import org.killbill.billing.util.cache.AccountBCDCacheLoader;
import org.killbill.billing.util.cache.Cachable.CacheType;
import org.killbill.billing.util.cache.CacheController;
import org.killbill.billing.util.cache.CacheControllerDispatcher;
import org.killbill.billing.util.cache.CacheLoaderArgument;
import org.killbill.billing.util.cache.ImmutableAccountCacheLoader.LoaderCallback;
import org.killbill.billing.util.dao.NonEntityDao;


import com.google.common.base.Function; import com.google.common.base.Function;
import com.google.common.collect.Collections2; import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;


public class DefaultAccountInternalApi implements AccountInternalApi { public class DefaultAccountInternalApi extends DefaultAccountApiBase implements AccountInternalApi {


private final AccountDao accountDao; private final AccountDao accountDao;
private final CacheControllerDispatcher cacheControllerDispatcher;
private final CacheController accountCacheController;
private final CacheController bcdCacheController;
private final NonEntityDao nonEntityDao;


@Inject @Inject
public DefaultAccountInternalApi(final AccountDao accountDao) { public DefaultAccountInternalApi(final AccountDao accountDao,
final NonEntityDao nonEntityDao,
final CacheControllerDispatcher cacheControllerDispatcher) {
super(accountDao, nonEntityDao, cacheControllerDispatcher);
this.accountDao = accountDao; this.accountDao = accountDao;
this.nonEntityDao = nonEntityDao;
this.cacheControllerDispatcher = cacheControllerDispatcher;
this.accountCacheController = cacheControllerDispatcher.getCacheController(CacheType.ACCOUNT_IMMUTABLE);
this.bcdCacheController = cacheControllerDispatcher.getCacheController(CacheType.ACCOUNT_BCD);
} }


@Override @Override
public Account getAccountById(final UUID accountId, final InternalTenantContext context) throws AccountApiException { public Account getAccountById(final UUID accountId, final InternalTenantContext context) throws AccountApiException {
final AccountModelDao account = accountDao.getById(accountId, context); return super.getAccountById(accountId, context);
if (account == null) { }
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_ID, accountId);
} @Override
return new DefaultAccount(account); public Account getAccountByKey(final String key, final InternalTenantContext context) throws AccountApiException {
return super.getAccountByKey(key, context);
} }


@Override @Override
public Account getAccountByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException { public Account getAccountByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException {
final AccountModelDao accountModelDao = getAccountModelDaoByRecordId(recordId, context); return super.getAccountByRecordId(recordId, context);
return new DefaultAccount(accountModelDao);
} }


@Override @Override
public void updateBCD(final String externalKey, final int bcd, public void updateBCD(final String externalKey, final int bcd,
final InternalCallContext context) throws AccountApiException { final InternalCallContext context) throws AccountApiException {
final Account currentAccount = getAccountByKey(externalKey, context); final Account currentAccount = getAccountByKey(externalKey, context);
if (currentAccount == null) { if (currentAccount == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_KEY, externalKey); throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_KEY, externalKey);
Expand All @@ -76,13 +96,15 @@ public void updateBCD(final String externalKey, final int bcd,
final MutableAccountData mutableAccountData = currentAccount.toMutableAccountData(); final MutableAccountData mutableAccountData = currentAccount.toMutableAccountData();
mutableAccountData.setBillCycleDayLocal(bcd); mutableAccountData.setBillCycleDayLocal(bcd);

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 2, 2015

Member

This section of code might be a bit dangerous.

Previously, junction would call updateAccount which would call mergeWithDelegate. The implementation has a safety check to ensure the BCD isn't updated. Now, we by-pass this check here, so we need to make sure DefaultInternalBillingApi knows when to update or not (using the magic 0 value?).

If not, we could be in a really weird state as if the account was created with the BCD set, putIfAbsent below wouldn't update the cache and we would be out-of-sync with the db...

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Oct 2, 2015

Author Member
  1. I changed the logic in updateAccount to NOT allow updating the BCD from api if this is already set
  2. Junction will only call the updateBCD, if it is not already set.

It is true that there is a race condition where while we compute our first invoice (and set BCD) through junction, we could have in parallel an account update trying to set that BCD (but this is not new). The only way to prevent that would be to enforce the check at the SQL level or take the global account lock when updating the account.

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 2, 2015

Member

I actually didn't have a race condition in mind (but it's a good point...), but wanted to point out that the 0 here is really critical (which maps to this). That's not new, but the previous updateAccount implementation would add a second check and throw an exception in case the BCD was set.

Just wondering if it's worth re-adding a safety check here before calling setBillCycleDayLocal.
final AccountModelDao accountToUpdate = new AccountModelDao(currentAccount.getId(), mutableAccountData); final AccountModelDao accountToUpdate = new AccountModelDao(currentAccount.getId(), mutableAccountData);
bcdCacheController.putIfAbsent(currentAccount.getId(), new Integer(bcd));
accountDao.update(accountToUpdate, context); accountDao.update(accountToUpdate, context);
} }


@Override @Override
public int getBCD(final UUID accountId, final InternalTenantContext context) throws AccountApiException { public int getBCD(final UUID accountId, final InternalTenantContext context) throws AccountApiException {
final Account account = getAccountById(accountId, context); final CacheLoaderArgument arg = createBCDCacheLoaderArgument(context);
return account.getBillCycleDayLocal(); final Integer result = (Integer) bcdCacheController.get(accountId, arg);
return result != null ? result : DefaultMutableAccountData.DEFAULT_BILLING_CYCLE_DAY_LOCAL;
} }


@Override @Override
Expand All @@ -97,15 +119,6 @@ public AccountEmail apply(final AccountEmailModelDao input) {
})); }));
} }


@Override
public Account getAccountByKey(final String key, final InternalTenantContext context) throws AccountApiException {
final AccountModelDao accountModelDao = accountDao.getAccountByKey(key, context);
if (accountModelDao == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_KEY, key);
}
return new DefaultAccount(accountModelDao);
}

@Override @Override
public void removePaymentMethod(final UUID accountId, final InternalCallContext context) throws AccountApiException { public void removePaymentMethod(final UUID accountId, final InternalCallContext context) throws AccountApiException {
updatePaymentMethod(accountId, null, context); updatePaymentMethod(accountId, null, context);
Expand All @@ -125,20 +138,14 @@ public UUID getByRecordId(final Long recordId, final InternalTenantContext conte


@Override @Override
public ImmutableAccountData getImmutableAccountDataById(final UUID accountId, final InternalTenantContext context) throws AccountApiException { public ImmutableAccountData getImmutableAccountDataById(final UUID accountId, final InternalTenantContext context) throws AccountApiException {
final Account account = getAccountById(accountId, context); final Long recordId = nonEntityDao.retrieveRecordIdFromObject(accountId, ObjectType.ACCOUNT, cacheControllerDispatcher.getCacheController(CacheType.RECORD_ID));
return new DefaultImmutableAccountData(account); return getImmutableAccountDataByRecordId(recordId, context);
}

@Override
public ImmutableAccountData getImmutableAccountDataByKey(final String key, final InternalTenantContext context) throws AccountApiException {
final Account account = getAccountByKey(key, context);
return new DefaultImmutableAccountData(account);
} }


@Override @Override
public ImmutableAccountData getImmutableAccountDataByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException { public ImmutableAccountData getImmutableAccountDataByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException {
final Account account = getAccountByRecordId(recordId, context); final CacheLoaderArgument arg = createImmutableAccountCacheLoaderArgument(context);
return new DefaultImmutableAccountData(account); return (ImmutableAccountData) accountCacheController.get(recordId, arg);
} }


private AccountModelDao getAccountModelDaoByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException { private AccountModelDao getAccountModelDaoByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException {
Expand All @@ -149,5 +156,36 @@ private AccountModelDao getAccountModelDaoByRecordId(final Long recordId, final
return accountModelDao; return accountModelDao;
} }


private int getBCDInternal(final UUID accountId, final InternalTenantContext context) {
final Long bcd = accountDao.getAccountBCD(accountId, context);
return bcd != null ? bcd.intValue() : DefaultMutableAccountData.DEFAULT_BILLING_CYCLE_DAY_LOCAL;
}

private CacheLoaderArgument createImmutableAccountCacheLoaderArgument(final InternalTenantContext context) {
final LoaderCallback loaderCallback = new LoaderCallback() {
@Override
public Object loadAccount(final Long recordId, final InternalTenantContext context) {
final Account account = getAccountByRecordIdInternal(recordId, context);
return account != null ? new DefaultImmutableAccountData(account) : null;
}
};
final Object[] args = new Object[1];
args[0] = loaderCallback;
final ObjectType irrelevant = null;
return new CacheLoaderArgument(irrelevant, args, context);
}


private CacheLoaderArgument createBCDCacheLoaderArgument(final InternalTenantContext context) {
final AccountBCDCacheLoader.LoaderCallback loaderCallback = new AccountBCDCacheLoader.LoaderCallback() {
@Override
public Object loadAccountBCD(final UUID accountId, final InternalTenantContext context) {
int bcd = getBCDInternal(accountId, context);
return new Integer(bcd);
}
};
final Object[] args = new Object[1];
args[0] = loaderCallback;
final ObjectType irrelevant = null;
return new CacheLoaderArgument(irrelevant, args, context);
}
} }
84 changes: 84 additions & 0 deletions account/src/main/java/org/killbill/billing/account/api/user/DefaultAccountApiBase.java
View file
@@ -0,0 +1,84 @@
/*
* Copyright 2014-2015 Groupon, Inc
* Copyright 2014-2015 The Billing Project, LLC
*
* The Billing Project licenses this file to you under the Apache License, version 2.0
* (the "License"); you may not use this file except in compliance with the
* License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package org.killbill.billing.account.api.user;

import java.util.UUID;

import org.killbill.billing.ErrorCode;
import org.killbill.billing.ObjectType;
import org.killbill.billing.account.api.Account;
import org.killbill.billing.account.api.AccountApiException;
import org.killbill.billing.account.api.DefaultAccount;
import org.killbill.billing.account.api.DefaultImmutableAccountData;
import org.killbill.billing.account.dao.AccountDao;
import org.killbill.billing.account.dao.AccountModelDao;
import org.killbill.billing.callcontext.InternalTenantContext;
import org.killbill.billing.util.cache.Cachable.CacheType;
import org.killbill.billing.util.cache.CacheController;
import org.killbill.billing.util.cache.CacheControllerDispatcher;
import org.killbill.billing.util.dao.NonEntityDao;

public class DefaultAccountApiBase {

private final AccountDao accountDao;
private final CacheControllerDispatcher cacheControllerDispatcher;
private final CacheController accountCacheController;
private final NonEntityDao nonEntityDao;

public DefaultAccountApiBase(final AccountDao accountDao,
final NonEntityDao nonEntityDao,
final CacheControllerDispatcher cacheControllerDispatcher) {
this.accountDao = accountDao;
this.nonEntityDao = nonEntityDao;
this.cacheControllerDispatcher = cacheControllerDispatcher;
this.accountCacheController = cacheControllerDispatcher.getCacheController(CacheType.ACCOUNT_IMMUTABLE);
}

protected Account getAccountById(final UUID accountId, final InternalTenantContext context) throws AccountApiException {
final Long recordId = nonEntityDao.retrieveRecordIdFromObject(accountId, ObjectType.ACCOUNT, cacheControllerDispatcher.getCacheController(CacheType.RECORD_ID));
final Account account = getAccountByRecordIdInternal(recordId, context);
if (account == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_ID, accountId);
}
accountCacheController.putIfAbsent(accountId, new DefaultImmutableAccountData(account));
return account;
}

protected Account getAccountByKey(final String key, final InternalTenantContext context) throws AccountApiException {
final AccountModelDao accountModelDao = accountDao.getAccountByKey(key, context);
if (accountModelDao == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_KEY, key);
}
final Account account = new DefaultAccount(accountModelDao);
accountCacheController.putIfAbsent(account.getId(), new DefaultImmutableAccountData(account));
return account;
}

protected Account getAccountByRecordId(final Long recordId, final InternalTenantContext context) throws AccountApiException {
final Account account = getAccountByRecordIdInternal(recordId, context);
if (account == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_RECORD_ID, recordId);
}
return account;
}

protected Account getAccountByRecordIdInternal(final Long recordId, final InternalTenantContext context) {
final AccountModelDao accountModelDao = accountDao.getByRecordId(recordId, context);
return accountModelDao != null ? new DefaultAccount(accountModelDao) : null;
}
}
44 changes: 23 additions & 21 deletions account/src/main/java/org/killbill/billing/account/api/user/DefaultAccountUserApi.java
View file
Expand Up @@ -32,9 +32,12 @@
import org.killbill.billing.account.dao.AccountDao; import org.killbill.billing.account.dao.AccountDao;
import org.killbill.billing.account.dao.AccountEmailModelDao; import org.killbill.billing.account.dao.AccountEmailModelDao;
import org.killbill.billing.account.dao.AccountModelDao; import org.killbill.billing.account.dao.AccountModelDao;
import org.killbill.billing.callcontext.InternalTenantContext;
import org.killbill.billing.util.cache.CacheControllerDispatcher;
import org.killbill.billing.util.callcontext.CallContext; import org.killbill.billing.util.callcontext.CallContext;
import org.killbill.billing.util.callcontext.InternalCallContextFactory; import org.killbill.billing.util.callcontext.InternalCallContextFactory;
import org.killbill.billing.util.callcontext.TenantContext; import org.killbill.billing.util.callcontext.TenantContext;
import org.killbill.billing.util.dao.NonEntityDao;
import org.killbill.billing.util.entity.Pagination; import org.killbill.billing.util.entity.Pagination;
import org.killbill.billing.util.entity.dao.DefaultPaginationHelper.SourcePaginationBuilder; import org.killbill.billing.util.entity.dao.DefaultPaginationHelper.SourcePaginationBuilder;


Expand All @@ -45,17 +48,35 @@


import static org.killbill.billing.util.entity.dao.DefaultPaginationHelper.getEntityPaginationNoException; import static org.killbill.billing.util.entity.dao.DefaultPaginationHelper.getEntityPaginationNoException;


public class DefaultAccountUserApi implements AccountUserApi { public class DefaultAccountUserApi extends DefaultAccountApiBase implements AccountUserApi {


private final InternalCallContextFactory internalCallContextFactory; private final InternalCallContextFactory internalCallContextFactory;
private final AccountDao accountDao; private final AccountDao accountDao;


@Inject @Inject
public DefaultAccountUserApi(final InternalCallContextFactory internalCallContextFactory, final AccountDao accountDao) { public DefaultAccountUserApi(final AccountDao accountDao,
final NonEntityDao nonEntityDao,
final CacheControllerDispatcher cacheControllerDispatcher,
final InternalCallContextFactory internalCallContextFactory) {
super(accountDao, nonEntityDao, cacheControllerDispatcher);
this.internalCallContextFactory = internalCallContextFactory; this.internalCallContextFactory = internalCallContextFactory;
this.accountDao = accountDao; this.accountDao = accountDao;
} }



@Override
public Account getAccountByKey(final String key, final TenantContext context) throws AccountApiException {
final InternalTenantContext internalTenantContext = internalCallContextFactory.createInternalTenantContext(context);
return getAccountByKey(key, internalTenantContext);
}

@Override
public Account getAccountById(final UUID id, final TenantContext context) throws AccountApiException {
final InternalTenantContext internalTenantContext = internalCallContextFactory.createInternalTenantContext(context);
return getAccountById(id, internalTenantContext);
}


@Override @Override
public Account createAccount(final AccountData data, final CallContext context) throws AccountApiException { public Account createAccount(final AccountData data, final CallContext context) throws AccountApiException {
// Not transactional, but there is a db constraint on that column // Not transactional, but there is a db constraint on that column
Expand All @@ -69,25 +90,6 @@ public Account createAccount(final AccountData data, final CallContext context)
return new DefaultAccount(account); return new DefaultAccount(account);

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 2, 2015

Member

Should we pre-populate the cache? It could be significant when using the Ruby client library for instance who will do auto-refreshes (create -> follow 201).

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Oct 2, 2015

Author Member

Not really because what we cache is the immutable part of the account and what we return is the full thing (which will always require a query).

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Oct 2, 2015

Member

Ah, yup, good point.
} }


@Override
public Account getAccountByKey(final String key, final TenantContext context) throws AccountApiException {
final AccountModelDao account = accountDao.getAccountByKey(key, internalCallContextFactory.createInternalTenantContext(context));
if (account == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_KEY, key);
}

return new DefaultAccount(account);
}

@Override
public Account getAccountById(final UUID id, final TenantContext context) throws AccountApiException {
final AccountModelDao account = accountDao.getById(id, internalCallContextFactory.createInternalTenantContext(context));
if (account == null) {
throw new AccountApiException(ErrorCode.ACCOUNT_DOES_NOT_EXIST_FOR_ID, id);
}

return new DefaultAccount(account);
}


@Override @Override
public Pagination<Account> searchAccounts(final String searchKey, final Long offset, final Long limit, final TenantContext context) { public Pagination<Account> searchAccounts(final String searchKey, final Long offset, final Long limit, final TenantContext context) {
Expand Down
18 changes: 10 additions & 8 deletions account/src/main/java/org/killbill/billing/account/dao/AccountDao.java
View file
Expand Up @@ -28,26 +28,28 @@


public interface AccountDao extends EntityDao<AccountModelDao, Account, AccountApiException> { public interface AccountDao extends EntityDao<AccountModelDao, Account, AccountApiException> {


public AccountModelDao getAccountByKey(String key, InternalTenantContext context); AccountModelDao getAccountByKey(String key, InternalTenantContext context);


public Pagination<AccountModelDao> searchAccounts(String searchKey, Long offset, Long limit, InternalTenantContext context); Pagination<AccountModelDao> searchAccounts(String searchKey, Long offset, Long limit, InternalTenantContext context);


/** /**
* @throws AccountApiException when externalKey is null * @throws AccountApiException when externalKey is null
*/ */
public UUID getIdFromKey(String externalKey, InternalTenantContext context) throws AccountApiException; UUID getIdFromKey(String externalKey, InternalTenantContext context) throws AccountApiException;


/** /**
* @param accountId the id of the account * @param accountId the id of the account
* @param paymentMethodId the is of the current default paymentMethod * @param paymentMethodId the is of the current default paymentMethod
*/ */
public void updatePaymentMethod(UUID accountId, UUID paymentMethodId, InternalCallContext context) throws AccountApiException; void updatePaymentMethod(UUID accountId, UUID paymentMethodId, InternalCallContext context) throws AccountApiException;


public void update(AccountModelDao account, InternalCallContext context) throws AccountApiException; void update(AccountModelDao account, InternalCallContext context) throws AccountApiException;


public void addEmail(AccountEmailModelDao email, InternalCallContext context) throws AccountApiException; void addEmail(AccountEmailModelDao email, InternalCallContext context) throws AccountApiException;


public void removeEmail(AccountEmailModelDao email, InternalCallContext context); void removeEmail(AccountEmailModelDao email, InternalCallContext context);


public List<AccountEmailModelDao> getEmailsByAccountId(UUID accountId, InternalTenantContext context); List<AccountEmailModelDao> getEmailsByAccountId(UUID accountId, InternalTenantContext context);

Long getAccountBCD(UUID accountId, InternalTenantContext context);
} }

0 comments on commit 8637993

Please sign in to comment.