Cache and Lazy Loading Related Modifications #1454
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a comprehensive enhancement to the entity loading and caching mechanism in CommCare's backend. The changes focus on improving performance and flexibility by introducing new attributes like Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodeEntityFactory
participant AsyncNodeEntityFactory
participant EntityLoadingProgressListener
participant AsyncEntity
Client->>NodeEntityFactory: setEntityProgressListener()
Client->>NodeEntityFactory: prepareEntities()
NodeEntityFactory->>AsyncNodeEntityFactory: primeCache()
AsyncNodeEntityFactory->>EntityLoadingProgressListener: publishProgress(PHASE_PROCESSING)
AsyncNodeEntityFactory->>AsyncEntity: calculateNonLazyFields()
AsyncEntity-->>AsyncNodeEntityFactory: Field Data
AsyncNodeEntityFactory->>EntityLoadingProgressListener: publishProgress(PHASE_CACHING)
AsyncNodeEntityFactory->>EntityLoadingProgressListener: publishProgress(PHASE_COMPLETE)
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…r fields marked as optimize
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/main/java/org/commcare/cases/entity/AsyncEntity.java (2)
179-179: Avoid usingprintStackTrace()in production codeUsing
xpe.printStackTrace();is not recommended in production environments. Consider removing it or replacing it with appropriate logging.Apply this diff to remove the unnecessary
printStackTrace():- xpe.printStackTrace();
256-256: RemoveprintStackTrace()from exception handlingDirect usage of
printStackTrace();should be avoided. Use the logging framework to log exceptions instead.Apply this diff to remove the
printStackTrace()call:- xpe.printStackTrace();src/main/java/org/commcare/cases/entity/EntityLoadingProgressListener.java (2)
25-25: Typo in documentation commentThere's a typo in the comment for
PHASE_UNCACHED_CALCULATION.Apply this diff to correct the spelling of "already":
- * already cached and similarly can be very quick when most things are arealdy cached. + * already cached and similarly can be very quick when most things are already cached.
52-52: Typo in Javadoc parameter descriptionThe word "corrosponding" should be "corresponding" in the method documentation.
Apply this diff to fix the typo:
- * @param progress progress corrosponding to the current entity loading phase + * @param progress progress corresponding to the current entity loading phasesrc/main/java/org/commcare/cases/entity/AsyncNodeEntityFactory.java (2)
Line range hint
82-100: Good improvements to primeCache method.The changes enhance the method with cancellation support and progress tracking. Consider adding error handling to report failures to the progress listener.
protected void primeCache() { if (isCancelled) return; if (progressListener != null) { progressListener.publishEntityLoadingProgress( EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_CACHING, 0, 100); } + try { if (mEntityCache == null || mTemplateIsCachable == null || !mTemplateIsCachable || mCacheHost == null) { return; } String[][] cachePrimeKeys = mCacheHost.getCachePrimeGuess(); if (cachePrimeKeys == null) { return; } mEntityCache.primeCache(mEntitySet,cachePrimeKeys, detail); + } catch (Exception e) { + if (progressListener != null) { + progressListener.publishEntityLoadingProgress( + EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_CACHING, -1, 100); + } + throw e; + } if (progressListener != null) { progressListener.publishEntityLoadingProgress( EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_CACHING, 100, 100); } }
Line range hint
133-178: Consider optimizing the nested loops for better performance.The implementation correctly handles cancellation and progress tracking. However, the nested loops in
setUnCachedDatacould be optimized for better performance.Consider these optimizations:
- Pre-calculate the number of fields to avoid repeated calls to
getNumFields()- Batch the progress updates to reduce overhead
protected void setUnCachedData(List<Entity<TreeReference>> entities) { + final int numFields = detail.getFields().length; + final int progressUpdateInterval = Math.max(1, entities.size() / 100); for (int i = 0; i < entities.size(); i++) { if (isCancelled) return; AsyncEntity e = (AsyncEntity)entities.get(i); - for (int col = 0; col < e.getNumFields(); ++col) { + for (int col = 0; col < numFields; ++col) { if (detail.getFields()[col].isOptimize()) { e.getField(col); e.getSortField(col); } } - if (progressListener != null) { + if (progressListener != null && i % progressUpdateInterval == 0) { progressListener.publishEntityLoadingProgress( EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_UNCACHED_CALCULATION, i, entities.size()); } } + if (progressListener != null) { + progressListener.publishEntityLoadingProgress( + EntityLoadingProgressListener.EntityLoadingProgressPhase.PHASE_UNCACHED_CALCULATION, + entities.size(), entities.size()); + } }src/main/java/org/commcare/cases/entity/NodeEntityFactory.java (1)
198-208: Well-structured base class methods.Good implementation of base methods with appropriate error handling. Consider adding Javadoc to document the expected behavior of these methods.
+/** + * Caches the provided entities. Default implementation throws RuntimeException. + * Subclasses should override this method if they support caching. + * + * @param entities List of entities to cache + * @throws RuntimeException if caching is not supported + */ public void cacheEntities(List<Entity<TreeReference>> entities) { throw new RuntimeException("Method not supported for normal Node Entity Factory"); } +/** + * Sets the progress listener for entity loading operations. + * + * @param progressListener The progress listener to use + */ public void setEntityProgressListener(EntityLoadingProgressListener progressListener) { this.progressListener = progressListener; } +/** + * Cancels the current loading operation. Default implementation throws RuntimeException. + * Subclasses should override this method if they support cancellation. + * + * @throws RuntimeException if cancellation is not supported + */ public void cancelLoading() { throw new RuntimeException("Method not supported for normal Node Entity Factory"); }src/test/java/org/commcare/backend/suite/model/test/AppStructureTests.java (1)
259-263: Consider adding more comprehensive test cases.While the current test covers the basic functionality, consider adding test cases for:
- Cache disabled scenarios
- Mixed optimize flags across fields
- Edge cases with empty or invalid attributes
@Test public void testDetailPerformanceAttributes_comprehensive() { // Test cache disabled Detail detailNoCaching = mApp.getSession().getPlatform().getDetail("m1_case_short"); assertFalse(detailNoCaching.isCacheEnabled()); // Test mixed optimize flags Detail detailMixedOptimize = mApp.getSession().getPlatform().getDetail("m2_case_short"); assertTrue(detailMixedOptimize.getFields()[0].isOptimize()); assertFalse(detailMixedOptimize.getFields()[1].isOptimize()); }src/main/java/org/commcare/suite/model/Detail.java (1)
518-528: Consider optimizing the field iteration.The method could be more efficient by short-circuiting when the first optimizable field is found.
Consider this optimization:
public boolean shouldOptimize() { if (cacheEnabled || lazyLoading) { for (DetailField field : fields) { if (field.isOptimize()) { - return true; + return true; // Early return on first optimizable field } - } + } } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/org/commcare/cases/entity/AsyncEntity.java(10 hunks)src/main/java/org/commcare/cases/entity/AsyncNodeEntityFactory.java(6 hunks)src/main/java/org/commcare/cases/entity/EntityLoadingProgressListener.java(1 hunks)src/main/java/org/commcare/cases/entity/EntityStorageCache.java(1 hunks)src/main/java/org/commcare/cases/entity/NodeEntityFactory.java(3 hunks)src/main/java/org/commcare/suite/model/Detail.java(8 hunks)src/main/java/org/commcare/suite/model/DetailField.java(5 hunks)src/main/java/org/commcare/xml/DetailFieldParser.java(1 hunks)src/main/java/org/commcare/xml/DetailParser.java(2 hunks)src/test/java/org/commcare/backend/suite/model/test/AppStructureTests.java(1 hunks)src/test/resources/app_structure/suite.xml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (26)
src/main/java/org/commcare/cases/entity/EntityStorageCache.java (2)
12-15: LGTM! Well-structured enum addition.The new
ValueTypeenum provides clear type safety for distinguishing between normal and sort fields. The naming is descriptive and follows Java conventions.
19-19: Verify implementations of breaking changes.The interface has two breaking changes:
- Added
ValueTypeparameter togetCacheKey- Renamed
getSortFieldIdFromCacheKeytogetFieldIdFromCacheKeyThese changes improve the API but require updates to all implementing classes.
Let's verify the implementations:
Also applies to: 25-25
src/main/java/org/commcare/cases/entity/AsyncEntity.java (6)
4-5: Imports added forValueTypeconstantsThe static imports for
TYPE_NORMAL_FIELDandTYPE_SORT_FIELDenhance code readability and maintainability.
53-53: Consistent addition ofcacheEnabledflagThe introduction of the
cacheEnabledboolean aligns with existing coding conventions and integrates well with the class's functionality.
Line range hint
74-94: Refactored constructor improves cohesionChanging the constructor to accept a
Detailobject instead of separate parameters simplifies the initialization process and improves cohesion betweenAsyncEntityandDetail.
98-118:calculateNonLazyFieldsmethod correctly processes field evaluationThe logic in
calculateNonLazyFieldsaccurately handles different scenarios based on caching and lazy loading configurations, ensuring that fields are evaluated appropriately.
138-145: Efficient field retrieval with proper cachingThe
getFieldmethod efficiently checks for existing data and evaluates non-optimized fields immediately, improving performance.
146-171: Proper synchronization and exception handling ingetFieldThe method correctly synchronizes access to shared resources and handles potential I/O exceptions, adhering to best practices for concurrent programming.
src/main/java/org/commcare/cases/entity/EntityLoadingProgressListener.java (1)
1-56: NewEntityLoadingProgressListenerinterface is well-designedThe interface effectively allows tracking of entity loading progress with clearly defined phases and documentation.
src/main/java/org/commcare/xml/DetailParser.java (2)
42-43: Parsing of new attributescache_enabledandlazy_loadingThe addition of parsing for
cache_enabledandlazy_loadingattributes enhances the configurability ofDetailobjects.
145-145: Updated constructor call forDetailIncluding
cacheEnabledin theDetailconstructor ensures that the caching configuration is properly propagated.src/main/java/org/commcare/cases/entity/AsyncNodeEntityFactory.java (3)
33-33: LGTM! Good addition of cancellation support.Adding cancellation support is a good practice for async operations to prevent unnecessary processing.
122-131: Well-structured caching logic.Good separation of concerns between new and legacy caching pathways based on the cache enabled flag.
59-60: Verify initialization of AsyncEntity with new parameters.The constructor now includes additional parameters for caching support. Ensure that all parameters are properly initialized in the AsyncEntity class.
✅ Verification successful
Constructor implementation verified and correct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check AsyncEntity constructor implementation ast-grep --pattern 'class AsyncEntity { $$$ AsyncEntity($$$) { $$$ } $$$ }'Length of output: 89
Script:
#!/bin/bash # Find the file containing AsyncEntity class rg "class AsyncEntity" -l # If found, examine the implementation echo "=== AsyncEntity Implementation ===" rg "class AsyncEntity" -A 50Length of output: 5295
src/main/java/org/commcare/cases/entity/NodeEntityFactory.java (1)
28-28: LGTM! Good encapsulation of progress listener.The protected field allows proper access in subclasses while maintaining encapsulation.
src/main/java/org/commcare/xml/DetailFieldParser.java (1)
34-35: LGTM! Consistent attribute parsing implementation.The parsing of the "optimize" attribute follows the established pattern. Verify that the XML schema documentation is updated to reflect this new attribute.
src/main/java/org/commcare/suite/model/DetailField.java (4)
80-81: LGTM: Field declaration follows Java conventions.The
optimizefield is properly declared as a private boolean field, following Java conventions.
306-308: LGTM: Standard getter implementation.The
isOptimize()method follows Java bean naming conventions for boolean getters.
446-448: LGTM: Builder pattern implementation.The
setOptimize()method in the Builder class follows the builder pattern correctly.
219-219: LGTM: Proper serialization handling.The
optimizefield is correctly handled in bothreadExternalandwriteExternalmethods usingExtUtil.Also applies to: 253-253
src/main/java/org/commcare/suite/model/Detail.java (4)
112-112: LGTM: Field declaration follows Java conventions.The
cacheEnabledfield is properly declared as a private boolean field.
130-130: LGTM: Constructor parameter properly handled.The
cacheEnabledparameter is correctly added and assigned.Also applies to: 182-182
238-240: LGTM: Standard getter implementation.The
isCacheEnabled()method follows Java bean naming conventions for boolean getters.
272-275: LGTM: Clear deprecation notice.The comment clearly explains that this is a legacy way to enable cache and index, helping future maintainers understand the transition.
src/test/resources/app_structure/suite.xml (2)
14-14: LGTM: Detail attributes properly configured.The
cache_enabledattribute is correctly added alongside the existinglazy_loadingattribute.
69-69: LGTM: Field optimization enabled.The
optimizeattribute is correctly added to the field element, which will trigger the optimization in the correspondingDetailFieldinstance.
shubham1g5
force-pushed
the
cacheAndIndexModifications
branch
from
0f48c78 to
2ec92ad
Compare
shubham1g5
changed the title
| private void primeCache() { | ||
| protected void primeCache() { | ||
| if (isCancelled) return; | ||
| if (progressListener != null) { |
There was a problem hiding this comment.
Will it cause issues for this progress update to occur before all of the null checks below that dump out of the method and/or without a finally {} clause? It seems it it would keep the listener spinning forever.
There was a problem hiding this comment.
These changes all seem clear / appropriate for the stated spec. Left a couple of points of feedback
Knowing that part of the goal here is long term to be managing entity loading over more flexible lifecycles, I'm a little worried that issues with managing the listener lifecycle here could be an issue.
| * @param progressListener The progress listener to use | ||
| */ | ||
| public void setEntityProgressListener(EntityLoadingProgressListener progressListener) { | ||
| this.progressListener = progressListener; |
There was a problem hiding this comment.
If there is an existing listener set, I think this method should close it out before changing the current one. Alternatively this method should potentially error out, or otherwise treat the listener as immutable once set.
Product Description
Supporting changes for dimagi/commcare-android#2928
AsyncEntityto support seletive caching and lazy-loading for only optimizable fieldsSafety Assurance
Safety story
Doesn't introduce any changes in existing behaviour
Automated test coverage
QA Plan
Will go through UAT but possibly after merge.
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates