Skip to content

Commit

Permalink
Attempt to fix #343 and #342 (re-opened)
Browse files Browse the repository at this point in the history 
Code is in place and test pass.
Need a second code review pass, and iron details about what should be a payment config property, versus hard-coded value
  • Loading branch information
@sbrossie
sbrossie committed Jul 10, 2015
1 parent 4feaf91 commit 16e2e51
Show file tree
Hide file tree
Showing 23 changed files with 450 additions and 91 deletions.
7 changes: 6 additions & 1 deletion api/src/main/java/org/killbill/billing/events/PaymentErrorInternalEvent.java
View file
Expand Up @@ -17,15 +17,20 @@


import java.util.UUID; import java.util.UUID;


import org.killbill.billing.payment.api.TransactionStatus;
import org.killbill.billing.payment.api.TransactionType; import org.killbill.billing.payment.api.TransactionType;


public interface PaymentErrorInternalEvent extends BusInternalEvent { public interface PaymentErrorInternalEvent extends PaymentInternalEvent {


public String getMessage(); public String getMessage();


public UUID getAccountId(); public UUID getAccountId();


public UUID getPaymentId(); public UUID getPaymentId();


public UUID getPaymentTransactionId();

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Jul 10, 2015

Member

Both shouldn't required anymore (present in PaymentInternalEvent).

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Jul 10, 2015

Author Member

Yes, i think i am gonna do a second pass on that because if we change the json of our payment events, we might as well do it well. But got your point.

public TransactionStatus getStatus();

public TransactionType getTransactionType(); public TransactionType getTransactionType();
} }
9 changes: 1 addition & 8 deletions api/src/main/java/org/killbill/billing/events/PaymentInfoInternalEvent.java
View file
Expand Up @@ -24,19 +24,12 @@
import org.killbill.billing.payment.api.TransactionStatus; import org.killbill.billing.payment.api.TransactionStatus;
import org.killbill.billing.payment.api.TransactionType; import org.killbill.billing.payment.api.TransactionType;


public interface PaymentInfoInternalEvent extends BusInternalEvent { public interface PaymentInfoInternalEvent extends PaymentInternalEvent {

public UUID getPaymentId();

public UUID getAccountId();


public BigDecimal getAmount(); public BigDecimal getAmount();


public Currency getCurrency(); public Currency getCurrency();


public DateTime getEffectiveDate(); public DateTime getEffectiveDate();


public TransactionStatus getStatus();

public TransactionType getTransactionType();
} }
37 changes: 37 additions & 0 deletions api/src/main/java/org/killbill/billing/events/PaymentInternalEvent.java
View file
@@ -0,0 +1,37 @@
/*
* 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.events;

import java.util.UUID;

import org.killbill.billing.payment.api.TransactionStatus;
import org.killbill.billing.payment.api.TransactionType;

public interface PaymentInternalEvent extends BusInternalEvent {

public UUID getAccountId();

public UUID getPaymentId();

public UUID getPaymentTransactionId();

public TransactionType getTransactionType();

public TransactionStatus getStatus();

}
14 changes: 2 additions & 12 deletions api/src/main/java/org/killbill/billing/events/PaymentPluginErrorInternalEvent.java
View file
Expand Up @@ -13,19 +13,9 @@
* License for the specific language governing permissions and limitations * License for the specific language governing permissions and limitations
* under the License. * under the License.
*/ */
package org.killbill.billing.events;

import java.util.UUID;


import org.killbill.billing.payment.api.TransactionType; package org.killbill.billing.events;

public interface PaymentPluginErrorInternalEvent extends BusInternalEvent {


public interface PaymentPluginErrorInternalEvent extends PaymentInternalEvent {
public String getMessage(); public String getMessage();

public UUID getAccountId();

public UUID getPaymentId();

public TransactionType getTransactionType();
} }
27 changes: 26 additions & 1 deletion payment/src/main/java/org/killbill/billing/payment/api/DefaultPaymentErrorEvent.java
View file
Expand Up @@ -30,11 +30,15 @@ public class DefaultPaymentErrorEvent extends BusEventBase implements PaymentErr
private final String message; private final String message;
private final UUID accountId; private final UUID accountId;
private final UUID paymentId; private final UUID paymentId;
private final UUID paymentTransactionId;
private final TransactionStatus status;
private final TransactionType transactionType; private final TransactionType transactionType;


@JsonCreator @JsonCreator
public DefaultPaymentErrorEvent(@JsonProperty("accountId") final UUID accountId, public DefaultPaymentErrorEvent(@JsonProperty("accountId") final UUID accountId,
@JsonProperty("paymentId") final UUID paymentId, @JsonProperty("paymentId") final UUID paymentId,
@JsonProperty("paymentTransactionId") final UUID paymentTransactionId,
@JsonProperty("status") final TransactionStatus status,
@JsonProperty("transactionType") final TransactionType transactionType, @JsonProperty("transactionType") final TransactionType transactionType,
@JsonProperty("message") final String message, @JsonProperty("message") final String message,
@JsonProperty("searchKey1") final Long searchKey1, @JsonProperty("searchKey1") final Long searchKey1,
Expand All @@ -44,6 +48,8 @@ public DefaultPaymentErrorEvent(@JsonProperty("accountId") final UUID accountId,
this.message = message; this.message = message;
this.accountId = accountId; this.accountId = accountId;
this.paymentId = paymentId; this.paymentId = paymentId;
this.paymentTransactionId = paymentTransactionId;
this.status = status;
this.transactionType = transactionType; this.transactionType = transactionType;
} }


Expand All @@ -67,6 +73,16 @@ public TransactionType getTransactionType() {
return transactionType; return transactionType;
} }


@Override
public UUID getPaymentTransactionId() {
return paymentTransactionId;
}

@Override
public TransactionStatus getStatus() {
return status;
}

@JsonIgnore @JsonIgnore
@Override @Override
public BusInternalEventType getBusEventType() { public BusInternalEventType getBusEventType() {
Expand All @@ -79,6 +95,8 @@ public String toString() {
sb.append("message='").append(message).append('\''); sb.append("message='").append(message).append('\'');
sb.append(", accountId=").append(accountId); sb.append(", accountId=").append(accountId);
sb.append(", paymentId=").append(paymentId); sb.append(", paymentId=").append(paymentId);
sb.append(", paymentTransactionId=").append(paymentTransactionId);
sb.append(", status=").append(status);
sb.append(", transactionType=").append(transactionType); sb.append(", transactionType=").append(transactionType);
sb.append('}'); sb.append('}');
return sb.toString(); return sb.toString();
Expand Down Expand Up @@ -107,7 +125,12 @@ public boolean equals(final Object o) {
if (paymentId != null ? !paymentId.equals(that.paymentId) : that.paymentId != null) { if (paymentId != null ? !paymentId.equals(that.paymentId) : that.paymentId != null) {
return false; return false;
} }

if (paymentTransactionId != null ? !paymentTransactionId.equals(that.paymentTransactionId) : that.paymentTransactionId != null) {
return false;
}
if (status != null ? !status.equals(that.status) : that.status != null) {
return false;
}
return true; return true;
} }


Expand All @@ -117,6 +140,8 @@ public int hashCode() {
result = 31 * result + (accountId != null ? accountId.hashCode() : 0); result = 31 * result + (accountId != null ? accountId.hashCode() : 0);
result = 31 * result + (transactionType != null ? transactionType.hashCode() : 0); result = 31 * result + (transactionType != null ? transactionType.hashCode() : 0);
result = 31 * result + (paymentId != null ? paymentId.hashCode() : 0); result = 31 * result + (paymentId != null ? paymentId.hashCode() : 0);
result = 31 * result + (paymentTransactionId != null ? paymentTransactionId.hashCode() : 0);
result = 31 * result + (status != null ? status.hashCode() : 0);
return result; return result;
} }
} }
21 changes: 20 additions & 1 deletion payment/src/main/java/org/killbill/billing/payment/api/DefaultPaymentInfoEvent.java
View file
Expand Up @@ -32,6 +32,7 @@ public class DefaultPaymentInfoEvent extends BusEventBase implements PaymentInfo


private final UUID accountId; private final UUID accountId;
private final UUID paymentId; private final UUID paymentId;
private final UUID paymentTransactionId;
private final BigDecimal amount; private final BigDecimal amount;
private final Currency currency; private final Currency currency;
private final TransactionStatus status; private final TransactionStatus status;
Expand All @@ -41,6 +42,7 @@ public class DefaultPaymentInfoEvent extends BusEventBase implements PaymentInfo
@JsonCreator @JsonCreator
public DefaultPaymentInfoEvent(@JsonProperty("accountId") final UUID accountId, public DefaultPaymentInfoEvent(@JsonProperty("accountId") final UUID accountId,
@JsonProperty("paymentId") final UUID paymentId, @JsonProperty("paymentId") final UUID paymentId,
@JsonProperty("paymentTransactionId") final UUID paymentTransactionId,
@JsonProperty("amount") final BigDecimal amount, @JsonProperty("amount") final BigDecimal amount,
@JsonProperty("currency") final Currency currency, @JsonProperty("currency") final Currency currency,
@JsonProperty("status") final TransactionStatus status, @JsonProperty("status") final TransactionStatus status,
Expand All @@ -54,6 +56,7 @@ public DefaultPaymentInfoEvent(@JsonProperty("accountId") final UUID accountId,
super(searchKey1, searchKey2, userToken); super(searchKey1, searchKey2, userToken);
this.accountId = accountId; this.accountId = accountId;
this.paymentId = paymentId; this.paymentId = paymentId;
this.paymentTransactionId = paymentTransactionId;
this.amount = amount; this.amount = amount;
this.currency = currency; this.currency = currency;
this.status = status; this.status = status;
Expand All @@ -63,6 +66,7 @@ public DefaultPaymentInfoEvent(@JsonProperty("accountId") final UUID accountId,


public DefaultPaymentInfoEvent(final UUID accountId, public DefaultPaymentInfoEvent(final UUID accountId,
final UUID paymentId, final UUID paymentId,
final UUID paymentTransactionId,
final BigDecimal amount, final BigDecimal amount,
final Currency currency, final Currency currency,
final TransactionStatus status, final TransactionStatus status,
Expand All @@ -71,7 +75,7 @@ public DefaultPaymentInfoEvent(final UUID accountId,
final Long searchKey1, final Long searchKey1,
final Long searchKey2, final Long searchKey2,
final UUID userToken) { final UUID userToken) {
this(accountId, paymentId, amount, currency, status, transactionType, null, null, this(accountId, paymentId, paymentTransactionId, amount, currency, status, transactionType, null, null,
effectiveDate, searchKey1, searchKey2, userToken); effectiveDate, searchKey1, searchKey2, userToken);
} }


Expand Down Expand Up @@ -107,6 +111,11 @@ public UUID getPaymentId() {
return paymentId; return paymentId;
} }


@Override
public UUID getPaymentTransactionId() {
return paymentTransactionId;
}

@Override @Override
public TransactionType getTransactionType() { public TransactionType getTransactionType() {
return transactionType; return transactionType;
Expand All @@ -123,6 +132,7 @@ public String toString() {
sb.append("DefaultPaymentInfoEvent"); sb.append("DefaultPaymentInfoEvent");
sb.append("{accountId=").append(accountId); sb.append("{accountId=").append(accountId);
sb.append(", paymentId=").append(paymentId); sb.append(", paymentId=").append(paymentId);
sb.append(", paymentTransactionId=").append(paymentTransactionId);
sb.append(", amount=").append(amount); sb.append(", amount=").append(amount);
sb.append(", currency=").append(currency); sb.append(", currency=").append(currency);
sb.append(", status=").append(status); sb.append(", status=").append(status);
Expand All @@ -143,6 +153,8 @@ public int hashCode() {
+ ((effectiveDate == null) ? 0 : effectiveDate.hashCode()); + ((effectiveDate == null) ? 0 : effectiveDate.hashCode());
result = prime * result result = prime * result
+ ((paymentId == null) ? 0 : paymentId.hashCode()); + ((paymentId == null) ? 0 : paymentId.hashCode());
result = prime * result
+ ((paymentTransactionId == null) ? 0 : paymentTransactionId.hashCode());
result = prime * result result = prime * result
+ ((currency == null) ? 0 : currency.hashCode()); + ((currency == null) ? 0 : currency.hashCode());
result = prime * result + ((status == null) ? 0 : status.hashCode()); result = prime * result + ((status == null) ? 0 : status.hashCode());
Expand Down Expand Up @@ -196,6 +208,13 @@ public boolean equals(final Object obj) {
} else if (!paymentId.equals(other.paymentId)) { } else if (!paymentId.equals(other.paymentId)) {
return false; return false;
} }
if (paymentTransactionId == null) {
if (other.paymentTransactionId != null) {
return false;
}
} else if (!paymentTransactionId.equals(other.paymentTransactionId)) {
return false;
}
if (currency == null) { if (currency == null) {
if (other.currency != null) { if (other.currency != null) {
return false; return false;
Expand Down
24 changes: 24 additions & 0 deletions payment/src/main/java/org/killbill/billing/payment/api/DefaultPaymentPluginErrorEvent.java
View file
Expand Up @@ -29,11 +29,15 @@ public class DefaultPaymentPluginErrorEvent extends BusEventBase implements Paym
private final String message; private final String message;
private final UUID accountId; private final UUID accountId;
private final UUID paymentId; private final UUID paymentId;
private final UUID paymentTransactionId;
private final TransactionStatus status;
private final TransactionType transactionType; private final TransactionType transactionType;


@JsonCreator @JsonCreator
public DefaultPaymentPluginErrorEvent(@JsonProperty("accountId") final UUID accountId, public DefaultPaymentPluginErrorEvent(@JsonProperty("accountId") final UUID accountId,
@JsonProperty("paymentId") final UUID paymentId, @JsonProperty("paymentId") final UUID paymentId,
@JsonProperty("paymentTransactionId") final UUID paymentTransactionId,
@JsonProperty("status") final TransactionStatus status,
@JsonProperty("transactionType") final TransactionType transactionType, @JsonProperty("transactionType") final TransactionType transactionType,
@JsonProperty("message") final String message, @JsonProperty("message") final String message,
@JsonProperty("searchKey1") final Long searchKey1, @JsonProperty("searchKey1") final Long searchKey1,
Expand All @@ -43,6 +47,8 @@ public DefaultPaymentPluginErrorEvent(@JsonProperty("accountId") final UUID acco
this.message = message; this.message = message;
this.accountId = accountId; this.accountId = accountId;
this.paymentId = paymentId; this.paymentId = paymentId;
this.paymentTransactionId = paymentTransactionId;
this.status = status;
this.transactionType = transactionType; this.transactionType = transactionType;
} }


Expand All @@ -66,6 +72,16 @@ public TransactionType getTransactionType() {
return transactionType; return transactionType;
} }


@Override
public UUID getPaymentTransactionId() {
return paymentTransactionId;
}

@Override
public TransactionStatus getStatus() {
return status;
}

@JsonIgnore @JsonIgnore
@Override @Override
public BusInternalEventType getBusEventType() { public BusInternalEventType getBusEventType() {
Expand Down Expand Up @@ -95,6 +111,12 @@ public boolean equals(final Object o) {
if (paymentId != null ? !paymentId.equals(that.paymentId) : that.paymentId != null) { if (paymentId != null ? !paymentId.equals(that.paymentId) : that.paymentId != null) {
return false; return false;
} }
if (paymentTransactionId != null ? !paymentTransactionId.equals(that.paymentTransactionId) : that.paymentTransactionId != null) {
return false;
}
if (status != null ? !status.equals(that.status) : that.status != null) {
return false;
}


return true; return true;
} }
Expand All @@ -105,6 +127,8 @@ public int hashCode() {
result = 31 * result + (accountId != null ? accountId.hashCode() : 0); result = 31 * result + (accountId != null ? accountId.hashCode() : 0);
result = 31 * result + (transactionType != null ? transactionType.hashCode() : 0); result = 31 * result + (transactionType != null ? transactionType.hashCode() : 0);
result = 31 * result + (paymentId != null ? paymentId.hashCode() : 0); result = 31 * result + (paymentId != null ? paymentId.hashCode() : 0);
result = 31 * result + (paymentTransactionId != null ? paymentTransactionId.hashCode() : 0);
result = 31 * result + (status != null ? status.hashCode() : 0);
return result; return result;
} }
} }
29 changes: 23 additions & 6 deletions ...l/billing/payment/bus/InvoiceHandler.java → ...g/payment/bus/PaymentBusEventHandler.java
View file
Expand Up @@ -18,6 +18,7 @@


package org.killbill.billing.payment.bus; package org.killbill.billing.payment.bus;


import java.io.IOException;
import java.math.BigDecimal; import java.math.BigDecimal;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.LinkedList; import java.util.LinkedList;
Expand All @@ -29,9 +30,12 @@
import org.killbill.billing.account.api.AccountInternalApi; import org.killbill.billing.account.api.AccountInternalApi;
import org.killbill.billing.callcontext.InternalCallContext; import org.killbill.billing.callcontext.InternalCallContext;
import org.killbill.billing.events.InvoiceCreationInternalEvent; import org.killbill.billing.events.InvoiceCreationInternalEvent;
import org.killbill.billing.events.PaymentInternalEvent;
import org.killbill.billing.payment.api.DefaultPaymentInfoEvent;
import org.killbill.billing.payment.api.PaymentApiException; import org.killbill.billing.payment.api.PaymentApiException;
import org.killbill.billing.payment.api.PluginProperty; import org.killbill.billing.payment.api.PluginProperty;
import org.killbill.billing.payment.core.PluginRoutingPaymentProcessor; import org.killbill.billing.payment.core.PluginRoutingPaymentProcessor;
import org.killbill.billing.payment.core.janitor.Janitor;
import org.killbill.billing.payment.invoice.InvoicePaymentRoutingPluginApi; import org.killbill.billing.payment.invoice.InvoicePaymentRoutingPluginApi;
import org.killbill.billing.util.UUIDs; import org.killbill.billing.util.UUIDs;
import org.killbill.billing.util.callcontext.CallContext; import org.killbill.billing.util.callcontext.CallContext;
Expand All @@ -46,26 +50,39 @@
import com.google.common.eventbus.Subscribe; import com.google.common.eventbus.Subscribe;
import com.google.inject.Inject; import com.google.inject.Inject;


public class InvoiceHandler { public class PaymentBusEventHandler {


private final AccountInternalApi accountApi; private final AccountInternalApi accountApi;
private final InternalCallContextFactory internalCallContextFactory; private final InternalCallContextFactory internalCallContextFactory;
private final PluginRoutingPaymentProcessor pluginRoutingPaymentProcessor; private final PluginRoutingPaymentProcessor pluginRoutingPaymentProcessor;
private final PaymentConfig paymentConfig; private final PaymentConfig paymentConfig;
private final Janitor janitor;


private static final Logger log = LoggerFactory.getLogger(InvoiceHandler.class); private static final Logger log = LoggerFactory.getLogger(PaymentBusEventHandler.class);


@Inject @Inject
public InvoiceHandler(final PaymentConfig paymentConfig, public PaymentBusEventHandler(final PaymentConfig paymentConfig,
final AccountInternalApi accountApi, final AccountInternalApi accountApi,
final PluginRoutingPaymentProcessor pluginRoutingPaymentProcessor, final PluginRoutingPaymentProcessor pluginRoutingPaymentProcessor,
final InternalCallContextFactory internalCallContextFactory) { final Janitor janitor,
final InternalCallContextFactory internalCallContextFactory) {
this.paymentConfig = paymentConfig; this.paymentConfig = paymentConfig;
this.accountApi = accountApi; this.accountApi = accountApi;
this.janitor = janitor;
this.internalCallContextFactory = internalCallContextFactory; this.internalCallContextFactory = internalCallContextFactory;
this.pluginRoutingPaymentProcessor = pluginRoutingPaymentProcessor; this.pluginRoutingPaymentProcessor = pluginRoutingPaymentProcessor;
} }


@AllowConcurrentEvents
@Subscribe
public void processPaymentEvent(final PaymentInternalEvent event) {
try {
janitor.processPaymentEvent(event);
} catch (IOException e) {

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Jul 10, 2015

Member

Looking at the call stack, I don't think IOException is still thrown.

Otherwise, it would be nice to push down the handling of such exceptions as low as possible, because at this point here, it's a bit unclear what to do (should we re-throw to try to process the event later?).

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Jul 10, 2015

Author Member

The IOException comes from the call recordFutureNotification, but you are correct that i did refactor the code and it now caught (because that same path is used for when we dispatch the bus event and later when we retry (for cases when we can't transition into a stable state).

I'd like to actually throw the IOException from the bus handler part, so the event is retried (probably useless, since next call we fail), BUT that will highlight the fact that event dispatching failed (versus ignoring it, and then it looks like we were able to process it).

I'll make the change, and if you disagree, we can re-open the discussion

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Jul 10, 2015

Author Member

Actually i am backing off on that and i will implement what you suggested but i filed #348

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Jul 10, 2015

Member

i filed #348

Perfect, that was also one of my questions when I read the code :-)
log.error("Failed to process payment event {}", e.toString());
}
}

@AllowConcurrentEvents @AllowConcurrentEvents
@Subscribe @Subscribe
public void processInvoiceEvent(final InvoiceCreationInternalEvent event) { public void processInvoiceEvent(final InvoiceCreationInternalEvent event) {
Expand Down

7 comments on commit 16e2e51

@pierre
Copy link
Member

@pierre pierre commented on 16e2e51 Jul 10, 2015

Choose a reason for hiding this comment

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

One thought regarding configuration: it seems it would be easy to have a configuration per plugin (from the payment, we can retrieve the payment method and hence the plugin name)? While this would not completely solve the problem to determine wether a payment is ACH pending or HPP pending in the case of Adyen for instance, it would help optimize certain scenarii (e.g. disable completely the polling for CyberSource in case the report API isn't configured).

@pierre
Copy link
Member

@pierre pierre commented on 16e2e51 Jul 10, 2015

Choose a reason for hiding this comment

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

Regarding terminal states, we currently don't have UNKNOWN_ABORTED/PENDING_ABORTED, i.e. it is possible to have payments stuck in UNKNOWN/PENDING forever. Would it make sense to force a transition from PENDING to UNKNOWN after the retries delay has expired (e.g. 7 days)? This would make it easy to spot problematic transactions to fix manually (all UNKNOWN ones).

@pierre
Copy link
Member

@pierre pierre commented on 16e2e51 Jul 10, 2015

Choose a reason for hiding this comment

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

Overall 👍!

@sbrossie
Copy link
Member Author

Choose a reason for hiding this comment

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

Github's comments are 'at least once' semantics, but more than once this is 'at least 3 times' semantics...

@sbrossie
Copy link
Member Author

Choose a reason for hiding this comment

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

One thought regarding configuration: it seems it would be easy to have a configuration per plugin (from the payment, we can retrieve the payment method and hence the plugin name)? While this would not completely solve the problem to determine wether a payment is ACH pending or HPP pending in the case of Adyen for instance, it would help optimize certain scenarii (e.g. disable completely the polling for CyberSource in case the report API isn't configured).

But does it mean adding a new payment plugin API? In some ways that seem related to #331

@sbrossie
Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding terminal states, we currently don't have UNKNOWN_ABORTED/PENDING_ABORTED, i.e. it is possible to have payments stuck in UNKNOWN/PENDING forever.

Yes. And we said that some human at some point should look for OLD UNKNOWN/PENDING and do something with it.

Would it make sense to force a transition from PENDING to UNKNOWN after the retries delay has expired (e.g. 7 days)? This would make it easy to spot problematic transactions to fix manually (all UNKNOWN ones).

I don't like too much because we can't then differentiate easily the between the original UNKNOWN and the original PENDING (unless of course we look at the history). I 'd be more inclined to introduce the new state UNKNOWN_ABORTED/PENDING_ABORTED in that case.

@pierre
Copy link
Member

@pierre pierre commented on 16e2e51 Jul 10, 2015

Choose a reason for hiding this comment

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

But does it mean adding a new payment plugin API?

No, I actually thought we could simply specify it via properties (e.g. org.killbill.billing.payment.killbill-adyen.janitorRetries=1,3,3,3,60,60,60,...). But I see your point, it should be provided by the plugin (it has the knowledge)... One way would be to make OSGIConfigPropertiesService r/w instead of r/o and the plugin could simply set these properties on startup. More thinking needed here...

I don't like too much because we can't then differentiate easily the between the original UNKNOWN and the original PENDING (unless of course we look at the history)

Yeah, I see. Scratch my comment!

Please sign in to comment.