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

invoice: insert invoice item and audits in batch #1091

Merged
merged 6 commits into from Feb 6, 2019

Conversation

Projects
None yet
2 participants
@pierre
Copy link
Member

pierre commented Feb 1, 2019

This should help performance when inserting lots of rows sequentially (e.g. large number of invoice items on a single invoice).

The administrator deploying Kill Bill should experiment setting on rewriteBatchedStatements in the JDBC url to see how it affects performance (note that it sets useServerPrepStmts to false).

@sbrossie FYI, I'm currently investigating how to remove the extra sqlDao.getById(entityId, context) query in the Audit codepath (see TODO below). But the code is quite delicate to review, so I'll open a separate PR for clarity.

pierre added some commits Feb 1, 2019

invoice: insert invoice item and audits in batch.
This should help performance when inserting lots of rows
sequentially (e.g. large number of invoice items on a single invoice).

The developer should experiment setting on rewriteBatchedStatements
in the JDBC url to see how it affects performance (note that it sets
useServerPrepStmts to false).

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

@pierre pierre requested a review from sbrossie Feb 1, 2019

Fix the lack of consistency with regards to how we annotate our SqlDa…
…o when invalidating rows (soft delete).

In some cases we use ChangeType.DELETE and in some other cases we use ChangeType.UPDATE. Not only this is not clean
but this also creates [snowflakes in our code](https://github.com/killbill/killbill/blob/killbill-0.20.6/util/src/main/java/org/killbill/billing/util/entity/dao/EntitySqlDaoWrapperInvocationHandler.java#L323).
@sbrossie
Copy link
Member

sbrossie left a comment

Looking good - let's review my comments maybe.

final TimeZoneAwareEntity accountModelDao = (TimeZoneAwareEntity) entityModelDao;
context = internalCallContextFactory.createInternalCallContext(accountModelDao, entityModelDao.getRecordId(), contextMaybeWithoutAccountRecordId);
} else {
context = contextMaybeWithoutAccountRecordId;

This comment has been minimized.

@sbrossie

sbrossie Feb 1, 2019

Member

⚠️ Couple of potential issues:

  1. Now that we create one context for all entities, are we sure that all entities will share the same accountRecordId - fortunately we might be ok because i don't think we have the concept of a bulk insert account, but still a bit dangerous
  2. I think looking at this level to compute the context based off the first element in the list is also a bit dangerous., but maybe ok.

We should clear 1. (whether this can ever happen). Assuming it does not: Either we were passed a contextMaybeWithoutAccountRecordId with a non null accountRecordId (we could check, and then there is nothing to compute), or we are left with the following 3 cases:

  1. We are doing an insert and at least one entity is of type TableName.ACCOUNT => Context needs to be recreated
  2. All objects are of types TENANT, TENANT_BROADCASTS or TENANT_KVS and accountRecordId does not make sense, nothing to do
  3. All other use cases, the contextMaybeWithoutAccountRecordId should actually already have an accountRecordId set -- probably adding a precondition would make sense.

This comment has been minimized.

@pierre

pierre Feb 4, 2019

Author Member

See e9679a9.

reHydratedEntity = deletedEntities.get(entityId);
} else {
// See note above regarding "markAsInactive" operations
// TODO Could we avoid this query?

This comment has been minimized.

@sbrossie

sbrossie Feb 1, 2019

Member

I think this fix should help by at least non running the query when we don't need to. fyi: circleci seems happy with the change.

Then, there is potentially still a space for doing one query to read them all -- when needed.

This comment has been minimized.

@pierre

pierre Feb 4, 2019

Author Member

I think this fix should help by at least non running the query when we don't need to. fyi: circleci seems happy with the change.

Merged as 4c70f00.

Then, there is potentially still a space for doing one query to read them all -- when needed.

Indeed - more changes are coming, just want to get this PR merged first (carefully tackling this piece by piece...) 😄

pierre added some commits Feb 4, 2019

invoice: don't retrieve created objects after batch operation
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
Merge remote-tracking branch 'origin/fix-for-deleted-change-type' int…
…o invoice-performance-bulk-inserts

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
util: harden context validation in EntitySqlDaoWrapperInvocationHandler
Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@sbrossie
Copy link
Member

sbrossie left a comment

👍

@sbrossie sbrossie merged commit 5d4e6fe into work-for-release-0.21.x Feb 6, 2019

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: integration-tests Your tests passed on CircleCI!
Details
ci/circleci: test-h2 Your tests passed on CircleCI!
Details
ci/circleci: test-mysql Your tests passed on CircleCI!
Details
ci/circleci: test-postgresql Your tests passed on CircleCI!
Details

@pierre pierre deleted the invoice-performance-bulk-inserts branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment