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.

Sign up for GitHub

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

Jump to bottom

RecordFileLogger performing far too many queries against t_entities #550

Merged
merged 7 commits into from
Feb 24, 2020
Merged

RecordFileLogger performing far too many queries against t_entities #550

merged 7 commits into from
Feb 24, 2020

Conversation

mike-burrage-hedera
Copy link
Contributor

Detailed description:

  • Cache (shard,realm,num) -> t_entities.id mappings in memory.
  • Configurable hedera.mirror.importer.parser.entityIdCacheSize defaulting to 100,000. Cache does not expire. LRU.
  • Cache is loaded on process start.
  • RecordFileLogger now utilizes this in-memory-cache for writing t_transaction columns for payer and node, and avoids entity lookups for topic/contract/file/account ids on non-cud operations (submitMessage, cryptotransfer, fileappend, etc)
    • Exception: Create/Update/Delete operations on entities still load entities from the EntitiesRepository (30 minute cache invalidation period).
  • Fix ContractCreateInstance creates entity for proxyAccountId if absent #533 by not creating entities with entity_num 0 (in the case of proxyAccountId during entity creation, this indicates that there should be no proxyAccount).

Which issue(s) this PR fixes:
Fixes #547
Fixes #533

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@mike-burrage-hedera mike-burrage-hedera added bug Type: Something isn't working P1 labels Feb 21, 2020
@mike-burrage-hedera mike-burrage-hedera added this to the Mirror 0.6.0 milestone Feb 21, 2020
@mike-burrage-hedera mike-burrage-hedera self-assigned this Feb 21, 2020
@mike-burrage-hedera mike-burrage-hedera marked this pull request as ready for review February 21, 2020 20:22
@mike-burrage-hedera mike-burrage-hedera changed the title WIP: RecordFileLogger performing far too many queries against t_entities RecordFileLogger performing far too many queries against t_entities Feb 21, 2020
@codecov
Copy link

codecov bot commented Feb 21, 2020 •
edited
Loading

Codecov Report

Merging #550 into master will not change coverage.
The diff coverage is 86.58%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #550   +/-   ##
=========================================
  Coverage     64.27%   64.27%           
  Complexity      163      163           
=========================================
  Files            95       95           
  Lines          3057     3057           
  Branches        370      370           
=========================================
  Hits           1965     1965           
  Misses          936      936           
  Partials        156      156
Impacted Files Coverage Δ Complexity Δ
...era/mirror/importer/config/CacheConfiguration.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...mirror/importer/parser/CommonParserProperties.java 93.33% <100%> (ø) 0 <0> (ø) ⬇️
...irror/importer/parser/record/RecordFileLogger.java 85.84% <100%> (ø) 0 <0> (ø) ⬇️
...or/importer/parser/record/EntityIdCacheLoader.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...a/mirror/importer/repository/EntityRepository.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...va/com/hedera/mirror/importer/domain/EntityId.java 66.66% <66.66%> (ø) 0 <0> (ø) ⬇️
...mporter/repository/EntityRepositoryCustomImpl.java 83.33% <83.33%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42a51ae...85f001c. Read the comment docs.

steven-sheehy
steven-sheehy requested changes Feb 21, 2020
View reviewed changes
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/domain/EntityId.java Outdated Show resolved Hide resolved
...ror-importer/src/main/java/com/hedera/mirror/importer/parser/record/EntityIdCacheLoader.java Outdated Show resolved Hide resolved
...ror-importer/src/main/java/com/hedera/mirror/importer/parser/record/EntityIdCacheLoader.java Outdated Show resolved Hide resolved
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/domain/EntityId.java Show resolved Hide resolved
...-mirror-importer/src/main/java/com/hedera/mirror/importer/repository/EntityIdRepository.java Outdated
import org.springframework.data.repository.PagingAndSortingRepository;

@CacheConfig(cacheNames = "entity_ids", cacheManager = CacheConfiguration.BIG_LRU_CACHE)
public interface EntityIdRepository extends PagingAndSortingRepository<EntityId, Long> {
Copy link
Member

Choose a reason for hiding this comment

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

Once you switch to interface projection, just move these methods into existing EntityRepository. You can't use class-level cacheconfig though, of course.

...mirror-importer/src/main/java/com/hedera/mirror/importer/parser/record/RecordFileLogger.java Show resolved Hide resolved
steven-sheehy
steven-sheehy reviewed Feb 21, 2020
View reviewed changes
...-mirror-importer/src/main/java/com/hedera/mirror/importer/repository/EntityIdRepository.java Outdated
default EntityId findOrCreateBy(long entityShard, long entityRealm, long entityNum, int entityTypeId) {
var found = findByNativeIds(entityShard, entityRealm, entityNum);
if (!found.isPresent()) {
return save(new EntityId(entityShard, entityRealm, entityNum, entityTypeId));
Copy link
Member

@steven-sheehy steven-sheehy Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure this is updating the cache? With Spring AOP you generally can't call proxied methods from inside a proxied method. It doesn't exit the class and come back in so it doesn't hit the cache decorator. You can increase spring cache logging to confirm.

apeksharma
apeksharma reviewed Feb 21, 2020
View reviewed changes
Copy link
Contributor

@apeksharma apeksharma left a comment

Choose a reason for hiding this comment

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

LGTM.
How can we test it? Maybe, if @cacheable returns same object on cache hit, then that equality can help write a test ? and expecting different object after clearing cache (if spring data allows clearing cache)?

...ror-importer/src/main/java/com/hedera/mirror/importer/parser/record/EntityIdCacheLoader.java Outdated Show resolved Hide resolved
@mike-burrage-hedera
Copy link
Contributor Author

@apeksharma To unit test the caching, we need to enable caching in the unit tests (currently off in test), fix the tests to clear out the cache in between and then we can unit test if the same object is returned.

@mike-burrage-hedera
Copy link
Contributor Author

Performance improvement 7.92s/60k transactions

steven-sheehy
steven-sheehy previously approved these changes Feb 24, 2020
View reviewed changes
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

@mike-burrage-hedera
Utilize an in-memory mapping of t_entities.id to (shard,realm,num) to…
81c35cc 
… speed up insert.

Transactions that create/update/delete entities still go through the repository causing blocking/serial queries during processing.

Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Make entityIdCacheSize configurable.
992fb02 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Bug fix
2bab98b 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Address code review comments.
b4460d0 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Log message fix.
8e5f589 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Use DTO projection.
245ed9b 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera
Add config property hedera.mirror.importer.parser.entityIdCacheSize
85f001c 
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@mike-burrage-hedera mike-burrage-hedera merged commit 0dd2663 into master Feb 24, 2020
@mike-burrage-hedera mike-burrage-hedera deleted the feature/logger-entities branch February 24, 2020 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apeksharma apeksharma apeksharma left review comments

@steven-sheehy steven-sheehy steven-sheehy approved these changes

@Nana-EC Nana-EC Nana-EC left review comments

Assignees

@mike-burrage-hedera mike-burrage-hedera

Labels
bug Type: Something isn't working P1
Projects
None yet
Milestone
Mirror 0.6.0
4 participants
@mike-burrage-hedera @apeksharma @steven-sheehy @Nana-EC