Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.20 maintenance fixes #1278

Closed
wants to merge 10 commits into from
Closed

0.20 maintenance fixes #1278

wants to merge 10 commits into from

Conversation

@pierre
Copy link
Member

pierre commented Feb 7, 2020

Fixes for #1273 and #1275.

pierre added 5 commits Feb 7, 2020
…catalog

This fixes #1275.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
This fixes #1273.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
This reverts commit a43d97b.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@pierre pierre requested a review from sbrossie Feb 7, 2020
@@ -164,6 +176,7 @@ public UsageKey apply(final Usage input) {
new ContiguousIntervalConsumableUsageInArrear(usage, accountId, invoiceId, rawSubscriptionUsage, existingTrackingIds, targetDate, rawUsageStartDate, usageDetailMode, internalTenantContext);

inFlightInArrearUsageIntervals.put(usageKey, existingInterval);
allSeenUnitTypes.addAll(existingInterval.getUnitTypes());

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 7, 2020

Member

Shouldn't this be reset for each billing event though?

Let's say we have 2 billing events with different usage types:
* efftDt1: b1 {t1, t2}

  • efftDt2: b2 {t2}

-> If we record usage type t1, after efftDt2 -> park?

@@ -180,6 +193,21 @@ public UsageKey apply(final Usage input) {
}
}
}

// Sanity check: https://github.com/killbill/killbill/issues/1275
for (final RawUsage rawUsage : rawSubscriptionUsage) {

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 7, 2020

Member

This code could probably be moved inside ContiguousIntervalUsageInArrear#getRolledUpUsage where we already iterate across all rawSubscriptionUsage elements?

This comment has been minimized.

Copy link
@pierre

pierre Feb 9, 2020

Author Member

I agree there is a bug in the scenario you mentioned above (I was able to reproduce it in a test case), but I'm struggle coming up with a fix.

First, to move the logic into getRolledUpUsage, this logic has to be moved to:

private List<RawUsage> filterInputRawUsage(final List<RawUsage> rawSubscriptionUsage) {
final Iterable<RawUsage> filteredList = Iterables.filter(rawSubscriptionUsage, new Predicate<RawUsage>() {
@Override
public boolean apply(final RawUsage input) {
return unitTypes.contains(input.getUnitType());
}
});
return ImmutableList.copyOf(filteredList);
}

If I do so though, as I'm iterating through the usage data in getRolledUpUsage, it's hard the filter the recorded usage by unit type as it's unclear if the unknown recorded data is either not defined in the catalog, or simply for another usage section (one billing event maps to several ContiguousIntervalUsageInArrear).

The only solution I can find would be to move my logic inside the for loop of computeInArrearUsageInterval (meaning I would need to expose getRolledUpUsage publicly and call it there to filter the usage by date).

Might be worth for you refreshing your memory on this and talk through it in more details, in case I'm missing something...

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 11, 2020

Member

I looked at it, and i think one issue is that code from 0.20 and 0.22 has diverged quite a bit: In particular, i had added a mechanism to link a transitionTime with its billingEvent: https://github.com/killbill/killbill/commit/48681d8821db97a0952482e6abf8a308dea2f918#diff-3e43568d6c03fc887b25fd5883cb0c3aR99

So, what i had in mind was:

  1. Compute the allSeenUnitTypes per billingEvent (similar to toBeClosed)
  2. Pass the allSeenUnitTypes to interval.addBillingEvent(event) and leverage the TransitionTime structure to also attach the allSeenUnitTypes

=> At this point in ContiguousIntervalUsageInArrear we have the mapping between a given
transition -> billingEvent -> allSeenUnitTypes and we should be able as we go through the RolledUpUsage and check wether we see entries contains unit that are not part of the allSeenUnitTypes (for that specific billingEvent)

But... this code is not available on 0.20 so that makes things a bit more complex...

This comment has been minimized.

Copy link
@pierre

pierre Feb 11, 2020

Author Member

=> At this point in ContiguousIntervalUsageInArrear we have the mapping between a given transition -> billingEvent -> allSeenUnitTypes and we should be able as we go through the RolledUpUsage and check wether we see entries contains unit that are not part of the allSeenUnitTypes (for that specific billingEvent)

This is not enough though because of the filterInputRawUsage I mentioned above: each interval knows about the billing events but not about other intervals (unknown unit type could be caused either by a missing entry in the catalog or because it is owned by another interval -- typically when you have both capacity and consumable for the same plan).

I tried a few approaches and finally settled on a mix of backport of your patch + removing the logic filterInputRawUsage. To be reviewed...

return super.getTags(invoice.getAccountId(), invoiceId, auditMode, includedDeleted, tenantContext);
// See https://github.com/killbill/killbill/issues/1273
final UUID accountId = AuditLevel.NONE.equals(auditMode.getLevel()) ? null : invoiceApi.getInvoice(invoiceId, tenantContext).getAccountId();
return super.getTags(accountId, invoiceId, auditMode, includedDeleted, tenantContext);

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 7, 2020

Member

Yeah, the change makes sense from a perf point of view - handling this Nullable accountId is not great though, but what to do...

pierre added 5 commits Feb 11, 2020
See #1278 (comment).

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
This reverts commit 19b338f.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@pierre pierre mentioned this pull request Feb 11, 2020
@pierre

This comment has been minimized.

Copy link
Member Author

pierre commented Feb 11, 2020

Created #1279 with a cleaner history.

@pierre pierre closed this Feb 11, 2020
@pierre pierre deleted the 0.20-maintenance-fixes branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.