Skip to content

Commit

Permalink
Code review for 951a786 (See re-opened #342)
Browse files Browse the repository at this point in the history 
Will address the rework on IncompletePaymentTransactionTask based on events and notificationQ in a subsequent commit
  • Loading branch information
@sbrossie
sbrossie committed Jul 9, 2015
1 parent d3c8799 commit 4feaf91
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 30 deletions.
6 changes: 3 additions & 3 deletions payment/src/main/java/org/killbill/billing/payment/core/PaymentProcessor.java
View file
Expand Up @@ -413,11 +413,11 @@ public boolean apply(final PaymentTransactionModelDao curPaymentTransactionModel
PaymentTransactionModelDao newPaymentTransactionModelDao = curPaymentTransactionModelDao;

final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin = findPaymentTransactionInfoPlugin(newPaymentTransactionModelDao, pluginTransactions);
if (paymentTransactionInfoPlugin != null && curPaymentTransactionModelDao.getTransactionStatus() == TransactionStatus.UNKNOWN) {
if (paymentTransactionInfoPlugin != null) {
// Make sure to invoke the Janitor task in case the plugin fixes its state on the fly
// See https://github.com/killbill/killbill/issues/341
final Boolean hasChanged = incompletePaymentTransactionTask.updatePaymentAndTransactionIfNeededWithAccountLock(newPaymentModelDao, newPaymentTransactionModelDao, paymentTransactionInfoPlugin, internalTenantContext);
if (hasChanged != null && hasChanged) {
final boolean hasChanged = incompletePaymentTransactionTask.updatePaymentAndTransactionIfNeededWithAccountLock(newPaymentModelDao, newPaymentTransactionModelDao, paymentTransactionInfoPlugin, internalTenantContext);
if (hasChanged) {
newPaymentModelDao = paymentDao.getPayment(newPaymentModelDao.getId(), internalTenantContext);
newPaymentTransactionModelDao = paymentDao.getPaymentTransaction(newPaymentTransactionModelDao.getId(), internalTenantContext);
}
Expand Down
8 changes: 4 additions & 4 deletions payment/src/main/java/org/killbill/billing/payment/core/janitor/CompletionTaskBase.java
View file
Expand Up @@ -109,16 +109,16 @@ public interface JanitorIterationCallback {
public <T> T doIteration();
}

protected <T> T doJanitorOperationWithAccountLock(final JanitorIterationCallback callback, final Long accountRecordId, final InternalTenantContext internalTenantContext) {
protected <T> T doJanitorOperationWithAccountLock(final JanitorIterationCallback callback, final InternalTenantContext internalTenantContext) {
GlobalLock lock = null;
try {
final Account account = accountInternalApi.getAccountByRecordId(accountRecordId, internalTenantContext);
final Account account = accountInternalApi.getAccountByRecordId(internalTenantContext.getAccountRecordId(), internalTenantContext);
lock = locker.lockWithNumberOfTries(LockerType.ACCNT_INV_PAY.toString(), account.getExternalKey(), ProcessorBase.NB_LOCK_TRY);
return callback.doIteration();
} catch (AccountApiException e) {
log.warn(String.format("Janitor failed to retrieve account with recordId %s", accountRecordId), e);
log.warn(String.format("Janitor failed to retrieve account with recordId %s", internalTenantContext.getAccountRecordId()), e);
} catch (LockFailedException e) {
log.warn(String.format("Janitor failed to grab account with recordId %s", accountRecordId), e);
log.warn(String.format("Janitor failed to lock account with recordId %s", internalTenantContext.getAccountRecordId()), e);
} finally {
if (lock != null) {
lock.release();
Expand Down
8 changes: 7 additions & 1 deletion ...src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentAttemptTask.java
View file
Expand Up @@ -58,6 +58,12 @@
*/
public class IncompletePaymentAttemptTask extends CompletionTaskBase<PaymentAttemptModelDao> {

//
// Each paymentAttempt *should* transition to a new state, so fetching a limited size will still allow us to progress (as opposed to fetching the same entries over and over)
// We also don't expect to see too many entries in the INIT state.
//
private final static long MAX_ATTEMPTS_PER_ITERATIONS = 1000L;

private final PluginRoutingPaymentAutomatonRunner pluginControlledPaymentAutomatonRunner;

@Inject
Expand All @@ -72,7 +78,7 @@ public IncompletePaymentAttemptTask(final InternalCallContextFactory internalCal

@Override
public Iterable<PaymentAttemptModelDao> getItemsForIteration() {
final Pagination<PaymentAttemptModelDao> incompleteAttempts = paymentDao.getPaymentAttemptsByStateAcrossTenants(retrySMHelper.getInitialState().getName(), getCreatedDateBefore(), 0L, Long.MAX_VALUE);
final Pagination<PaymentAttemptModelDao> incompleteAttempts = paymentDao.getPaymentAttemptsByStateAcrossTenants(retrySMHelper.getInitialState().getName(), getCreatedDateBefore(), 0L, MAX_ATTEMPTS_PER_ITERATIONS);
if (incompleteAttempts.getTotalNbRecords() > 0) {
log.info("Janitor AttemptCompletionTask start run: found {} incomplete attempts", incompleteAttempts.getTotalNbRecords());
}
Expand Down
55 changes: 34 additions & 21 deletions ...main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
View file
Expand Up @@ -23,14 +23,11 @@
import javax.inject.Inject;

import org.joda.time.DateTime;
import org.killbill.billing.ErrorCode;
import org.killbill.billing.account.api.AccountApiException;
import org.killbill.billing.account.api.AccountInternalApi;
import org.killbill.billing.callcontext.InternalCallContext;
import org.killbill.billing.callcontext.InternalTenantContext;
import org.killbill.billing.catalog.api.Currency;
import org.killbill.billing.osgi.api.OSGIServiceRegistration;
import org.killbill.billing.payment.api.PaymentApiException;
import org.killbill.billing.payment.api.PluginProperty;
import org.killbill.billing.payment.api.TransactionStatus;
import org.killbill.billing.payment.core.PaymentTransactionInfoPluginConverter;
Expand Down Expand Up @@ -60,6 +57,7 @@

public class IncompletePaymentTransactionTask extends CompletionTaskBase<PaymentTransactionModelDao> {


private static final ImmutableList<TransactionStatus> TRANSACTION_STATUSES_TO_CONSIDER = ImmutableList.<TransactionStatus>builder()
.add(TransactionStatus.PENDING)
.add(TransactionStatus.UNKNOWN)
Expand All @@ -74,7 +72,8 @@ public IncompletePaymentTransactionTask(final InternalCallContextFactory interna

@Override
public Iterable<PaymentTransactionModelDao> getItemsForIteration() {
final Pagination<PaymentTransactionModelDao> result = paymentDao.getByTransactionStatusAcrossTenants(TRANSACTION_STATUSES_TO_CONSIDER, getCreatedDateBefore(), getCreatedDateAfter(), 0L, Long.MAX_VALUE);
// Code will go away in the next commit -- allow to fix h2...
final Pagination<PaymentTransactionModelDao> result = paymentDao.getByTransactionStatusAcrossTenants(TRANSACTION_STATUSES_TO_CONSIDER, getCreatedDateBefore(), getCreatedDateAfter(), 0L, 1000L);
if (result.getTotalNbRecords() > 0) {
log.info("Janitor IncompletePaymentTransactionTask start run: found {} pending/unknown payments", result.getTotalNbRecords());
}
Expand All @@ -88,19 +87,23 @@ public void doIteration(final PaymentTransactionModelDao paymentTransaction) {
doJanitorOperationWithAccountLock(new JanitorIterationCallback() {
@Override
public Void doIteration() {

// State may have changed since we originally retrieved with no lock
final PaymentTransactionModelDao rehydratedPaymentTransaction = paymentDao.getPaymentTransaction(paymentTransaction.getId(), internalTenantContext);

final TenantContext tenantContext = internalCallContextFactory.createTenantContext(internalTenantContext);
final PaymentModelDao payment = paymentDao.getPayment(paymentTransaction.getPaymentId(), internalTenantContext);
final PaymentModelDao payment = paymentDao.getPayment(rehydratedPaymentTransaction.getPaymentId(), internalTenantContext);

final PaymentMethodModelDao paymentMethod = paymentDao.getPaymentMethod(payment.getPaymentMethodId(), internalTenantContext);
final PaymentPluginApi paymentPluginApi = getPaymentPluginApi(payment, paymentMethod.getPluginName());

final PaymentTransactionInfoPlugin undefinedPaymentTransaction = new DefaultNoOpPaymentInfoPlugin(payment.getId(),
paymentTransaction.getId(),
paymentTransaction.getTransactionType(),
paymentTransaction.getAmount(),
paymentTransaction.getCurrency(),
paymentTransaction.getCreatedDate(),
paymentTransaction.getCreatedDate(),
rehydratedPaymentTransaction.getId(),
rehydratedPaymentTransaction.getTransactionType(),
rehydratedPaymentTransaction.getAmount(),
rehydratedPaymentTransaction.getCurrency(),
rehydratedPaymentTransaction.getCreatedDate(),
rehydratedPaymentTransaction.getCreatedDate(),
PaymentPluginStatus.UNDEFINED,
null);
PaymentTransactionInfoPlugin paymentTransactionInfoPlugin;
Expand All @@ -109,7 +112,7 @@ public Void doIteration() {
paymentTransactionInfoPlugin = Iterables.tryFind(result, new Predicate<PaymentTransactionInfoPlugin>() {
@Override
public boolean apply(final PaymentTransactionInfoPlugin input) {
return input.getKbTransactionPaymentId().equals(paymentTransaction.getId());
return input.getKbTransactionPaymentId().equals(rehydratedPaymentTransaction.getId());
}
}).or(new Supplier<PaymentTransactionInfoPlugin>() {
@Override
Expand All @@ -120,30 +123,38 @@ public PaymentTransactionInfoPlugin get() {
} catch (final Exception e) {
paymentTransactionInfoPlugin = undefinedPaymentTransaction;
}
updatePaymentAndTransactionIfNeeded(payment, paymentTransaction, paymentTransactionInfoPlugin, internalTenantContext);
updatePaymentAndTransactionIfNeeded(payment, rehydratedPaymentTransaction, paymentTransactionInfoPlugin, internalTenantContext);
return null;
}
}, paymentTransaction.getAccountRecordId(), internalTenantContext);
}, internalTenantContext);

}

public Boolean updatePaymentAndTransactionIfNeededWithAccountLock(final PaymentModelDao payment, final PaymentTransactionModelDao paymentTransaction, final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin, final InternalTenantContext internalTenantContext) {
return doJanitorOperationWithAccountLock(new JanitorIterationCallback() {
public boolean updatePaymentAndTransactionIfNeededWithAccountLock(final PaymentModelDao payment, final PaymentTransactionModelDao paymentTransaction, final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin, final InternalTenantContext internalTenantContext) {
// Can happen in the GET case, see PaymentProcessor#toPayment
if (!TRANSACTION_STATUSES_TO_CONSIDER.contains(paymentTransaction.getTransactionStatus())) {
// Nothing to do
return false;
}
final Boolean result = doJanitorOperationWithAccountLock(new JanitorIterationCallback() {
@Override
public Boolean doIteration() {
return updatePaymentAndTransactionIfNeeded(payment, paymentTransaction, paymentTransactionInfoPlugin, internalTenantContext);
return updatePaymentAndTransactionInternal(payment, paymentTransaction, paymentTransactionInfoPlugin, internalTenantContext);
}
}, internalTenantContext.getAccountRecordId(), internalTenantContext);
}, internalTenantContext);
return result != null && result;
}

private boolean updatePaymentAndTransactionIfNeeded(final PaymentModelDao payment, final PaymentTransactionModelDao paymentTransaction, final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin, final InternalTenantContext internalTenantContext) {
final CallContext callContext = createCallContext("IncompletePaymentTransactionTask", internalTenantContext);

// Can happen in the GET case, see PaymentProcessor#toPayment
if (!TRANSACTION_STATUSES_TO_CONSIDER.contains(paymentTransaction.getTransactionStatus())) {
// Nothing to do
return false;
}
return updatePaymentAndTransactionInternal(payment, paymentTransaction, paymentTransactionInfoPlugin, internalTenantContext);
}

private boolean updatePaymentAndTransactionInternal(final PaymentModelDao payment, final PaymentTransactionModelDao paymentTransaction, final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin, final InternalTenantContext internalTenantContext) {
final CallContext callContext = createCallContext("IncompletePaymentTransactionTask", internalTenantContext);

// First obtain the new transactionStatus,
// Then compute the new paymentState; this one is mostly interesting in case of success (to compute the lastSuccessPaymentState below)
Expand Down Expand Up @@ -189,8 +200,10 @@ private boolean updatePaymentAndTransactionIfNeeded(final PaymentModelDao paymen
paymentTransaction.getId(), transactionStatus, processedAmount, processedCurrency, gatewayErrorCode, gatewayError, internalCallContext);

return true;

}


// Keep the existing currentTransactionStatus if we can't obtain a better answer from the plugin; if not, return the newTransactionStatus
private TransactionStatus computeNewTransactionStatusFromPaymentTransactionInfoPlugin(final PaymentTransactionInfoPlugin input, final TransactionStatus currentTransactionStatus) {
final TransactionStatus newTransactionStatus = PaymentTransactionInfoPluginConverter.toTransactionStatus(input);
Expand Down
2 changes: 1 addition & 1 deletion payment/src/test/java/org/killbill/billing/payment/dao/TestPaymentDao.java
View file
Expand Up @@ -496,7 +496,7 @@ public void testPaginationForPaymentByStatesAcrossTenants() {
paymentDao.insertPaymentWithFirstTransaction(paymentModelDao1, transaction1, context1);
}

final Pagination<PaymentTransactionModelDao> result = paymentDao.getByTransactionStatusAcrossTenants(ImmutableList.of(TransactionStatus.UNKNOWN), clock.getUTCNow(), createdDate1, 0L, Long.MAX_VALUE);
final Pagination<PaymentTransactionModelDao> result = paymentDao.getByTransactionStatusAcrossTenants(ImmutableList.of(TransactionStatus.UNKNOWN), clock.getUTCNow(), createdDate1, 0L, new Long(NB_ENTRIES));
Assert.assertEquals(result.getTotalNbRecords(), new Long(NB_ENTRIES));

final Iterator<PaymentTransactionModelDao> iterator = result.iterator();
Expand Down

1 comment on commit 4feaf91

@pierre
Copy link
Member

@pierre pierre commented on 4feaf91 Jul 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Please sign in to comment.