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

Cleanup legacy caching code #1092

Merged
merged 3 commits into from Feb 8, 2019

Conversation

Projects
None yet
2 participants
@pierre
Copy link
Member

pierre commented Feb 7, 2019

  • EntitySqlDaoWrapperInvocationHandler#invokeWithCaching was only used for
    EntitySqlDao#getRecordId. Simplify the code by using NonEntityDao directly
    from EntityDaoBase, instead of relying on annotations. The @Cachable annotation isn't really used anymore.
  • Remove audit caches: these were actually never plugged properly (this makes #247 obsolete).

pierre added some commits Feb 5, 2019

util: remove EntityDao#getRecordId API
This wasn't used.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
util: remove snowflake code in EntitySqlDaoWrapperInvocationHandler
EntitySqlDaoWrapperInvocationHandler#invokeWithCaching was only used for
EntitySqlDao#getRecordId. Simplify the code by using NonEntityDao directly
from EntityDaoBase, instead of relying on annotations.

The @cachable annotation isn't really used anymore.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
util: remove audit caches
These were actually never plugged properly.

This makes #247 obsolete.

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

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

@@ -126,7 +140,16 @@ public M inTransaction(final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFacto
}

protected boolean checkEntityAlreadyExists(final EntitySqlDao<M, E> transactional, final M entity, final InternalCallContext context) {
return transactional.getRecordId(entity.getId().toString(), context) != null;
if (entity.getTableName().getObjectType() == null) {

This comment has been minimized.

@sbrossie

sbrossie Feb 7, 2019

Member

Maybe a Precondition instead ?

This comment has been minimized.

@pierre

pierre Feb 8, 2019

Author Member

We can't, as it's an expected case (for TenantBroadcastDao#create for instance).

Entities without an ObjectType are history tables, InvoiceTracking, InvoiceParent, NodeInfo, TenantBroadcasts and RolledUpUsage. In these cases, we always want to create the record. Worst case, we might get an index violation from the database (IIRC checkEntityAlreadyExists is mainly useful for tags and custom fields, where the check is smarter than just checking the id).

This comment has been minimized.

@sbrossie

sbrossie Feb 8, 2019

Member

Maybe i was not clear; Yes, it is an expected case to have entity with a null objectType. The question is whether this code path even makes sense to be called for such entity, and is so, what does the return value false really mean in that context?

This comment has been minimized.

@pierre

pierre Feb 11, 2019

Author Member

checkEntityAlreadyExists is always called for create DAO calls. When there is no objectType, the id doesn't mean anything (just a row identifier) and false means "always create the entry".

@@ -222,97 +210,6 @@ public Object execute() throws Throwable {
});
}

private Object invokeWithCaching(final Cachable cachableAnnotation, final Method method, final Object[] args)

This comment has been minimized.

@sbrossie

sbrossie Feb 7, 2019

Member

I think you mentionned our strategy for caching audit logs was busted, but it seems this path was much more than audit logs:

    String RECORD_ID_CACHE_NAME = "record-id";
    String ACCOUNT_RECORD_ID_CACHE_NAME = "account-record-id";
    String TENANT_RECORD_ID_CACHE_NAME = "tenant-record-id";
    String OBJECT_ID_CACHE_NAME = "object-id";
    String AUDIT_LOG_CACHE_NAME = "audit-log";
    String AUDIT_LOG_VIA_HISTORY_CACHE_NAME = "audit-log-via-history";
    String TENANT_CATALOG_CACHE_NAME = "tenant-catalog";
    String TENANT_PAYMENT_STATE_MACHINE_CONFIG_CACHE_NAME = "tenant-payment-state-machine-config";
    String TENANT_OVERDUE_CONFIG_CACHE_NAME = "tenant-overdue-config";
    String TENANT_CONFIG_CACHE_NAME = "tenant-config";
    String TENANT_KV_CACHE_NAME = "tenant-kv";
    String TENANT_CACHE_NAME = "tenant";
    String OVERRIDDEN_PLAN_CACHE_NAME = "overridden-plan";
    String ACCOUNT_IMMUTABLE_CACHE_NAME = "account-immutable";
    String ACCOUNT_BCD_CACHE_NAME = "account-bcd";
    String ACCOUNT_ID_FROM_BUNDLE_ID_CACHE_NAME = "account-id-from-bundle-id";
    String BUNDLE_ID_FROM_SUBSCRIPTION_ID_CACHE_NAME = "bundle-id-from-subscription-id";

I am all about simplifications but since we are talking about inserting entries in the cache, none of our testing will fail after removing that code, however, does it mean it was really useless?

This comment has been minimized.

@pierre

pierre Feb 8, 2019

Author Member

Here's my understanding of the current code (let me know if I'm missing something, it's a bit tricky...):

  • invokeWithCaching is invoked only if @Cachable annotation is present
    • IIRC we used this annotation more in the past, but now we invoke the caches directly as the stuff we cache is more complex (e.g. see InternalCallContextFactory)
  • @Cachable is annotated only on AuditSqlDao and EntitySqlDao methods
    1. For AuditSqlDao, getObjectType() would return null (as AuditSqlDao doesn't extend EntitySqlDao)
    2. For EntitySqlDao, getRecordId was only used in checkEntityAlreadyExists (fixed by invoking the cache directly)

=> Hence, it seems largely unused.

Note that I believe we could maybe fix 1. above, by extracting the tableName (so the ObjectType) from the arguments of the AuditSqlDao methods (instead of looking at the class hierarchy), but I'm not sure we should bother. The multi-node scenario would still be broken and we somehow passed all of our load tests so far without this caching working...

This comment has been minimized.

@sbrossie

sbrossie Feb 8, 2019

Member

All good. Agreed it's a path that seems unused and less code is better...

@sbrossie sbrossie merged commit dbc5a6e into work-for-release-0.21.x Feb 8, 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-v3 branch Feb 11, 2019

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