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 #1279

Merged
merged 7 commits into from Feb 14, 2020
Merged

0.20 maintenance fixes #1279

merged 7 commits into from Feb 14, 2020

Conversation

@pierre
Copy link
Member

pierre commented Feb 11, 2020

Fixes for #1273 and #1275.

See #1278.

pierre added 3 commits Feb 7, 2020
…catalog

This fixes #1275.

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>
@pierre pierre requested a review from sbrossie Feb 11, 2020
@pierre pierre mentioned this pull request Feb 11, 2020
}

// Build the usage interval that are no longer referenced
for (final UsageKey usageKey : toBeClosed) {
final ContiguousIntervalUsageInArrear interval = inFlightInArrearUsageIntervals.remove(usageKey);
if (interval != null) {
interval.addBillingEvent(event);
// No usage section for CANCEL events
interval.addAllSeenUnitTypesForBillingEvent(event, usages.isEmpty() ? allSeenUnitTypesForPrevBillingEvent : allSeenUnitTypesForBillingEvent);

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 11, 2020

Member

Confused about that one.. I would expect that we pass an empty set for any cancelation, ... I would also expect that for such events usages is empty (all the time)

}

@Test(groups = "slow", description = "https://github.com/killbill/killbill/issues/1275")
public void testWithUnknownUsage() throws Exception {

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 11, 2020

Member

Looks like this test relies on multiple catalog versions, it would make it (much) easier to point in the test which catalog version is used at a certain time (and possibly valid units)

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@pierre

This comment has been minimized.

Copy link
Member Author

pierre commented Feb 12, 2020

@sbrossie Updated as discussed (FYI it turns out I was already testing the case where usage is deleted in my new test).

Copy link
Member

sbrossie left a comment

I just want to validate the following is correct in the code -- i have been thinking about this last billing event CANCEL and why there is a dissymmetry... There should not be:

Let's say we have two billing events, b1 and b2, possibly with some transitions (e.g each month):

b1-- t1 --------- t2 -------- t3 -----b2

We will bill in-arrear at t1, t2, t3, and b2 (cancelation, change plan, ..). The allSeenUnitTypes should always refer to the one associated with the previous billing event. So in this scenario, all periods should look at b1 -> allSeenUnitTypes.

If we now have a b3 later on, are we still guaranteed to build [t3 - b2] using b1 -> allSeenUnitTypes, or will we use b2->allSeenUnitTypes` ?

@@ -81,7 +82,8 @@

protected final List<TransitionTime> transitionTimes;
protected final List<BillingEvent> billingEvents;
protected final Map<BillingEvent, Set<String>> allSeenUnitTypes;
// Ordering is important here!
protected final LinkedHashMap<BillingEvent, Set<String>> allSeenUnitTypes;

This comment has been minimized.

Copy link
@sbrossie

sbrossie Feb 13, 2020

Member

Do we really need this ordering? It seems all you care about is the ability to find the last billing event in the method addAllSeenUnitTypesFromPrevBillingEvent ? Shouldn't such last billing event be retrievable from billingEvents ?

This comment has been minimized.

Copy link
@pierre

pierre Feb 13, 2020

Author Member

Yes, we could look it up that way. But in that case, the ordering here needs to be swapped: https://github.com/killbill/killbill/pull/1279/files#diff-050672ee3314d0f9a29f5065b853673bR200 (call addAllSeenUnitTypesFromPrevBillingEvent before addBillingEvent). This felt a bit more sneaky...

@sbrossie

This comment has been minimized.

Copy link
Member

sbrossie commented Feb 13, 2020

@pierre I think we are close enough, that if you want to start looking at the merge, this should be fine...

@pierre

This comment has been minimized.

Copy link
Member Author

pierre commented Feb 13, 2020

i have been thinking about this last billing event CANCEL and why there is a dissymmetry...

It's not so much that the CANCEL event is dissymmetric (my original comment was wrong to point it out explicitly), it's more that the code is dissymmetric:

  • in the branch where existingInterval == null, we are creating a new interval because we discovered a new usage section. At this point, we start tracking the unit types seen.
  • in the for loop final UsageKey usageKey : toBeClosed, we are closing intervals which aren't referenced anymore by this billing event. This could be because of a CANCEL event (no more usage), or because we have a catalog version change (even if the usage hasn't changed, a new interval is created as UsageKey takes into account the catalog effective date). In either case, we need to look at the usage as defined by the previous billing event.

Does this make sense? If so, let me know if your concerned is addressed or if I should add a new test case to be safe (I didn't quite follow your example though 😬).

@pierre I think we are close enough, that if you want to start looking at the merge, this should be fine...

I'm going to release 0.20.15 as to unblock folks impacted by this, but I'll keep this PR open until we finally converge 😄

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@pierre

This comment has been minimized.

Copy link
Member Author

pierre commented Feb 13, 2020

@sbrossie I've added the following (passing) test: f184f37. Let me know if this is aligned with what you had in mind.

@pierre pierre merged commit 49469c0 into backport-0.20.x Feb 14, 2020
1 check failed
1 check failed
ci/circleci: build Your tests failed on CircleCI
Details
@pierre pierre deleted the 0.20-maintenance-fixes-rebased 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

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