chore: introduce caching and projection to optimise FPL#36118
chore: introduce caching and projection to optimise FPL#36118
Conversation
subrata71
requested review from
a team,
nidhi-nair and
sondermanish
as code owners
github-actions
bot
added
Backend
WalkthroughThe changes involve enhancements across multiple service and repository classes within the application. Key modifications include the addition of new parameters to existing methods, allowing for more precise data retrieval. A caching mechanism has been integrated to optimize performance when fetching application IDs associated with base page IDs, particularly in view mode. Additionally, test cases have been updated to align with these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConsolidatedAPIService
participant CacheManager
participant NewPageService
Client->>ConsolidatedAPIService: getConsolidatedInfoForPageLoad(basePageId, null, branchName, ApplicationMode.VIEW)
ConsolidatedAPIService->>CacheManager: check cache for application ID
alt Cache hit
CacheManager-->>ConsolidatedAPIService: return application ID
ConsolidatedAPIService-->>Client: return application details
else Cache miss
ConsolidatedAPIService->>NewPageService: findByBranchNameAndBasePageId(branchName, basePageId, null, null)
NewPageService-->>ConsolidatedAPIService: return application ID
ConsolidatedAPIService->>CacheManager: update cache with application ID
ConsolidatedAPIService-->>Client: return application details
end
Assessment against linked issues
Tip New featuresWalkthrough comment now includes:
Notes:
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (1)
35-36: Excellent update to thefindPageByBranchNameAndBasePageIdmethod! 🌟The addition of the
projectedFieldNamesparameter aligns perfectly with the newfindByIdmethod, allowing the caller to specify the desired fields to be included in the result. This enhancement improves the method's flexibility and efficiency.To further improve the code readability, consider adding a line break after the method declaration to separate it from the parameters, like this:
Mono<NewPage> findPageByBranchNameAndBasePageId( String branchName, String basePageId, AclPermission permission, List<String> projectedFieldNames );app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (1)
Verify the complete removal of the
findByIdAndBranchNamemethod.The
findByIdAndBranchNamemethod is still present in other parts of the codebase, including test files and other service implementations. This suggests that its removal fromNewPageServiceCE.javamight not be fully intentional or complete. Please ensure that all references to this method are updated or removed to prevent potential inconsistencies or errors.
- Files with remaining references:
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.javaAnalysis chain
Line range hint
1-1: Verify the impact of removing thefindByIdAndBranchNamemethod.The removal of this method indicates a change in how pages are retrieved by their ID and branch name. This could potentially affect any existing functionality that relied on this method.
Run the following script to verify the usage of the removed method:
If the method is still being used in other parts of the codebase, it could lead to compilation errors or unexpected behavior. Please ensure that all references to this method have been properly updated or removed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the removed `findByIdAndBranchName` method. # Test: Search for the method usage. Expect: No occurrences of the method. rg --type java -A 5 $'findByIdAndBranchName'Length of output: 2731
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (1)
43-50: Excellent work on the newfindByIdmethod! The code is clean and well-structured.A few points to note:
- The method signature clearly defines the expected parameters.
- The query builder is used effectively to construct the query based on the provided criteria.
- The permission is set to enforce access control.
- The projected fields are specified to control the fields included in the result.
One suggestion for improvement:
Consider adding error handling to handle cases where the provided ID is invalid or the
NewPageis not found. You can use theswitchIfEmptyoperator to throw a custom exception or return a default value in such cases.For example:
return queryBuilder() .criteria(Bridge.equal(NewPage.Fields.id, id)) .permission(permission) .fields(projectedFields) .one() .switchIfEmpty(Mono.error(new NewPageNotFoundException(id)));app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java (1)
110-111: Good work on optimizing the page retrieval! Let's verify if this change is sufficient.I see that you've updated the
findByBranchNameAndBasePageIdmethod call to include a list of fields to be retrieved. This is a great step towards optimizing the data retrieval process by fetching only the required fields.To ensure that this change is sufficient to achieve the optimization goal, please consider the following:
- Verify that the
idfield is the only field required for further processing in this method and its callers. If additional fields are needed, include them in theList.of()argument.- Confirm that the
findByBranchNameAndBasePageIdmethod supports the fields projection correctly and returns only the specified fields.- Assess the impact of this change on the overall performance of the application, especially in scenarios where this method is invoked frequently.
If you need any assistance with the verification process or have any questions, feel free to reach out. Keep up the good optimization work!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (14)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryTest.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java (1 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (1)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Additional comments not posted (27)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (1)
15-15: Great work adding the newfindByIdmethod! 👍The method signature looks perfect. It allows retrieving a
NewPageentity by ID while considering access permissions and specifying the desired fields to be included in the result. This granular retrieval capability enhances the flexibility and efficiency of the repository.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (1)
72-73: Great work on enhancing thefindByBranchNameAndBasePageIdmethod!The addition of the
projectedFieldNamesparameter is a clever way to optimize data retrieval by allowing the caller to specify which fields to include in the response. This can lead to more efficient network usage and faster response times.To ensure that this change is being utilized effectively, please verify the following:
Check if the
projectedFieldNamesparameter is being used correctly within the method implementation to filter the returned fields.Verify that the callers of this method are providing the appropriate field names when invoking the method.
You can use the following script to search for the method usage and analyze how the new parameter is being utilized:
If you find any instances where the
projectedFieldNamesparameter is not being used optimally or if there are discrepancies in how it's being passed at the call sites, please update the code accordingly.app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryTest.java (1)
104-104: Clarify the purpose and impact of the additional parameter in thefindPageByBranchNameAndBasePageIdmethod.Hello! I noticed that the
findPageByBranchNameAndBasePageIdmethod call in thefindPageWithoutBranchNametest case has been updated to include a newnullparameter at the end. This change suggests that the method signature in theCustomNewPageRepositoryinterface has been modified to accept an additional parameter.Could you please provide more information about the purpose and impact of this additional parameter? Understanding the reasoning behind this change will help in assessing its appropriateness and identifying any potential issues or improvements.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (1)
Line range hint
173-193: The changes to thefindPageByBranchNameAndBasePageIdmethod look good! The addition of theprojectedFieldNamesparameter enhances the flexibility of the method.Key points:
- The new parameter allows the caller to specify which fields to include in the result.
- The
fieldsmethod is used appropriately to pass the projected fields to the query builder.- The existing functionality of the method remains intact.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (2)
87-100: Excellent work on introducing theviewModeDefaultPageIdToDefaultAppIdLRUCachecache! 👏The cache field is well-declared with appropriate access modifiers and is properly initialized using Guava's
Cachewith an LRU eviction policy. The chosen maximum size of 330,000 entries is well-documented, and the reasoning behind the capacity is clearly explained.This cache will effectively optimize the retrieval of application IDs associated with default page IDs in view mode, reducing the need for repeated queries to the application service.
Great job on implementing this caching mechanism to enhance performance! 🚀
216-253: Great job on incorporating the caching logic into thegetConsolidatedInfoForPageLoadmethod! 🎉The modifications made to the method effectively utilize the
viewModeDefaultPageIdToDefaultAppIdLRUCachecache to optimize the retrieval of application IDs in view mode. The code handles both cache miss and cache hit scenarios appropriately, ensuring efficient retrieval of application details.In the case of a cache miss, the code falls back to querying the new page service to obtain the application ID and updates the cache with the new mapping for future requests. This approach minimizes unnecessary service calls and improves performance.
The caching mechanism is seamlessly integrated into the existing flow of the method without disrupting other parts of the code. The code is well-structured, readable, and includes clear comments explaining the purpose of each section.
Excellent work on implementing this caching optimization! It will significantly enhance the efficiency of retrieving application IDs in view mode. 👍
app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (2)
509-516: Great work on enhancing the method signature and functionality! 👍The changes to include the
projectedFieldNamesparameter and handle the case whenbranchNameis empty look good. This will allow callers to specify which fields they want to retrieve, potentially improving performance by reducing the amount of data processed and returned.Keep up the good work! 🌟
541-542: Nice update to pass specific field names! 👌Passing a specific list of field names to
findByBranchNameAndBasePageIdis a good change. It will retrieve only the required fields, potentially improving performance by reducing the amount of data processed and returned.Well done! 😊
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (4)
193-194: Good work! The updated method call matches the new signature offindByBranchNameAndBasePageIdinNewPageService. The test should continue to function as expected.
227-228: Looks good! ThefindByBranchNameAndBasePageIdmethod call has been correctly updated to match the new signature. The test case should continue to pass.
285-286: Great job! The method call tofindByBranchNameAndBasePageIdhas been properly updated to include the additional parameter, aligning with the new signature. The test should continue to function correctly.
381-382: Excellent! ThefindByBranchNameAndBasePageIdmethod call has been correctly updated to match the new signature that includes an additional parameter. This change ensures the test case remains valid.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (5)
227-227: Looks good!The code change aligns with the listed alteration and is likely part of the refactoring effort for the
getConsolidatedInfoForPageLoadmethod.
409-409: Good update!Updating the mock application ID helps differentiate it for different test scenarios.
415-415: Looks good!The updates to the mock page's application ID and the
findByBranchNameAndBasePageIdmethod call are consistent with the previous changes.Also applies to: 418-418
510-511: Good catch!Updating the page ID argument in the
getConsolidatedInfoForPageLoadmethod call ensures consistency with the mock page ID used in the test.
724-725: Great test case!Mocking the repository methods to return empty responses helps validate the error handling and response when an anonymous user tries to access a private application.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1)
866-866: Good catch! The test code has been updated to match the new method signature.The code change looks good. It correctly passes
nullas the fourth argument to align with the updatedfindByBranchNameAndBasePageIdmethod signature in theNewPageServiceclass.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java (1)
370-370: Great job testing the scenario of creating a CRUD page on a different branch! 👍The test method is well-structured, follows the AAA pattern, and uses the
StepVerifierto verify the results in a reactive way. The test covers an important scenario of creating a CRUD page on a different branch of a git connected application.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (2)
314-314: Verify the method signature change andnullparameter.The
findByBranchNameAndBasePageIdmethod is now being called with an additionalnullparameter. Please verify if this change to the method signature is intentional and thenullvalue is appropriate here.
315-317: Good usage of theMIGRATE_DSLspan for tracking performance.Using the
MIGRATE_DSLspan to track the performance of this code block is a good practice. The changes look fine.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
1007-1007: Verify method signature and handle null argument appropriately.The
findByBranchNameAndBasePageIdmethod call has been updated to include an additionalnullargument. Please ensure the following:
Verify if the method signature in
NewPageServicehas been updated to accept this additional argument. If not, this would lead to compilation errors.Clarify the purpose and impact of passing
nullas the argument. Arbitrarily passingnullcould lead to unexpected behavior if not handled properly within the method implementation.Within the
findByBranchNameAndBasePageIdmethod, check fornulland handle it appropriately to avoid potential null pointer exceptions or other issues.To verify the concerns, run the following script:
Verification successful
Let's gather more information to ensure we have a complete understanding of the situation. We'll run additional scripts to verify the method signature and check for null handling within the
NewPageService.javafile.
Ensure null handling for
projectedFieldNamesparameter.The method
findByBranchNameAndBasePageIdinNewPageServiceCEImpl.javahas been correctly updated to accept four parameters, including theprojectedFieldNameslist. However, it is important to implement null handling for this parameter within the method to avoid potential null pointer exceptions. Consider adding a check to handle cases whereprojectedFieldNamesmight be null.
- Location:
NewPageServiceCEImpl.java- Ensure null handling forprojectedFieldNames.By addressing this, you can prevent unexpected behavior and enhance the robustness of your code. Keep up the good work and continue to pay attention to these details!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the method signature and null handling for `findByBranchNameAndBasePageId`. # Test 1: Check if the method signature has been updated to accept the additional argument. # Expected result: The method should accept 4 arguments, with the new argument at the 3rd position. ast-grep --lang java --pattern $'findByBranchNameAndBasePageId($_, $_, $_, $_)' # Test 2: Check if the method implementation handles null for the new argument. # Expected result: There should be a null check for the 3rd argument. rg --type java -A 5 $'if \(\w+ == null\)' $(fd NewPageService.java$)Length of output: 247
Script:
#!/bin/bash # Description: Verify the existence and parameters of the method `findByBranchNameAndBasePageId`. # Search for the method definition to confirm its parameters. rg --type java 'findByBranchNameAndBasePageId' -A 3 # Check for null handling logic within the method implementation. rg --type java 'findByBranchNameAndBasePageId' -A 20 | rg 'if \(\w+ == null\)'Length of output: 12083
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (5)
1713-1713: Great test method for cloning an application with Git metadata!This test method thoroughly verifies the cloning of an application with Git metadata. It checks the important properties and associations of the cloned application, such as policies, pages, and other related entities.
The test method covers the following key aspects:
- Cloning the application with Git metadata
- Verifying the properties of the cloned application
- Checking the policies and permissions of the cloned application
- Ensuring the pages and other related entities are properly cloned
Overall, this test method provides good coverage for the cloning functionality of an application with Git metadata.
Line range hint
2540-2540: Excellent test method for handling the case when no applications are present in a workspace!This test method verifies the behavior of the
findByWorkspaceIdAndBaseApplicationsInRecentlyUsedOrdermethod when no applications are present in a workspace. It creates a new workspace for the user and calls the method to retrieve the applications.The test method asserts that an empty list is returned when no applications are found in the workspace. This is the expected behavior in this scenario.
The test method also includes proper cleanup by archiving the created workspace after the test.
Great job covering this edge case scenario!
Line range hint
2564-2564: Fantastic test method for verifying the ordering of applications based on recently used apps!This test method covers the scenario when applications are present in a workspace and recently used apps data exists for the user. It sets up the necessary test data by creating dummy applications in the workspace and configuring the recently used app data for the user.
The test method then calls the
findByWorkspaceIdAndBaseApplicationsInRecentlyUsedOrdermethod and verifies that the returned list of applications is ordered based on the recently used apps. It asserts that the size of the returned list matches the expected count and that the order of the application IDs matches the recently used app order.The test method also includes proper cleanup by deleting the created applications and archiving the workspace after the test.
Excellent work in covering this important scenario and verifying the expected behavior!
Line range hint
2600-2600: Excellent test method for handling the case when recently used apps data is absent!This test method covers the scenario when applications are present in a workspace but recently used apps data is not available for the user. It creates dummy applications in the workspace but does not set up any recently used app data.
The test method calls the
findByWorkspaceIdAndBaseApplicationsInRecentlyUsedOrdermethod and verifies that all the applications in the workspace are returned. It asserts that the size of the returned list matches the total number of applications and that all the application IDs are present in the returned list.The test method also includes proper cleanup by deleting the created applications and archiving the workspace after the test.
Great job in covering this scenario and ensuring that all applications are returned when recently used apps data is absent!
Line range hint
2635-2635: Great test method for handling an invalid workspace ID scenario!This test method verifies the behavior of the
findByWorkspaceIdAndBaseApplicationsInRecentlyUsedOrdermethod when an invalid workspace ID is provided. It calls the method with a randomly generated invalid workspace ID.The test method uses the
StepVerifierto assert that anAppsmithExceptionis thrown by the method. It verifies that the exception message matches the expected error message indicating that the workspace resource is not found.This test method ensures that the method handles the case of an invalid workspace ID gracefully and throws the appropriate exception.
Excellent work in covering this error scenario and verifying the expected exception behavior!
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
...server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Show resolved
Hide resolved
...psmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java
Outdated
Show resolved
Hide resolved
...er/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Outdated
Show resolved
Hide resolved
subrata71
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java (1)
Ensure Effective Utilization of
CacheManagerThe
CacheManageris currently only being passed to the constructor ofConsolidatedAPIServiceImplwithout further usage within the class. To fully leverage the benefits of caching, it is crucial to integrateCacheManagerinto the class's operations. Consider reviewing the class to identify areas where caching can be applied effectively.
- Location to Review:
ConsolidatedAPIServiceImpl.javaAnalysis chain
Line range hint
37-56: Good integration ofCacheManagerin the constructor.The addition of
CacheManageras a parameter in the constructor is a strategic move to enhance caching mechanisms within the service. This should ideally lead to performance improvements as outlined in the PR objectives. However, it would be prudent to ensure thatCacheManageris being utilized effectively throughout the class and not just being passed around.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `CacheManager` in the class. # Test: Search for the usage of `CacheManager` in the class. Expect: Multiple usages of `CacheManager`. rg --type java -A 5 $'CacheManager' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.javaLength of output: 700
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java (1)
Ensure Utilization of CacheManager in ApplicationPageServiceCEImpl
The
CacheManageris introduced in theApplicationPageServiceCEImplclass but is not actively used in any methods. This could indicate an incomplete integration or a placeholder for future development. It's important to ensure thatCacheManageris utilized effectively to achieve the intended caching benefits. Please verify its integration and consider implementing its usage where necessary to optimize performance.
- Location:
ApplicationPageServiceCEImplclass inapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.javaAnalysis chain
Line range hint
64-95: Ensure proper integration of CacheManager.The addition of
CacheManagerto the constructor is a significant change aimed at enhancing caching capabilities. It's crucial to ensure that this new dependency is properly integrated and utilized within the class to achieve the desired performance improvements. Please verify that theCacheManageris being used effectively in other parts of the class or related classes to optimize data retrieval and storage operations.
Let's explore further to ensure that the
CacheManageris being effectively utilized. We need to check if it's being used in the superclassApplicationPageServiceCEImplor any related components. This will help us understand if the integration is complete or if there might be an oversight. Let's run a script to search for its usage in the superclass.
Let's try a broader search to ensure we haven't missed anything. We'll look for any usage of
CacheManagerin the entireApplicationPageServiceCEImplclass. This will help us confirm whether it's being utilized or if there's an oversight. Let's proceed with a new script.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of CacheManager in the class and related classes. # Test: Search for the usage of CacheManager in the class and related classes. Expect: Usage of CacheManager methods. rg --type java -A 5 $'CacheManager'Length of output: 23102
Script:
#!/bin/bash # Description: Verify the usage of CacheManager in the ApplicationPageServiceCEImpl class. # Test: Search for the usage of CacheManager in the ApplicationPageServiceCEImpl class. Expect: Usage of CacheManager methods. rg --type java -A 5 $'class ApplicationPageServiceCEImpl' -g '*.java' | rg 'CacheManager'Length of output: 88
Script:
#!/bin/bash # Description: Verify the usage of CacheManager in the ApplicationPageServiceCEImpl class. # Test: Search for the usage of CacheManager in the ApplicationPageServiceCEImpl class. Expect: Usage of CacheManager methods. rg --type java -A 10 'CacheManager' -g '*ApplicationPageServiceCEImpl.java'Length of output: 3584
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ApplicationSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java
Additional comments not posted (3)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ApplicationSpanCE.java (1)
8-9: Well done on adding new constants for Redis operations.The addition of
APPLICATION_ID_FETCH_REDIS_SPANandAPPLICATION_ID_UPDATE_REDIS_SPANenhances the observability within Redis operations, which is crucial for monitoring and debugging. The naming convention is consistent and clear, making the code easy to understand and maintain.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java (1)
Line range hint
40-59: Proper integration ofCacheManagerin the base class constructor.Introducing
CacheManagerin the constructor ofConsolidatedAPIServiceCECompatibleImplis a commendable approach to ensure that caching capabilities are available throughout the derived classes. This aligns well with the PR's objectives to optimize performance. It is important to verify that this change is consistently propagated to all subclasses to maintain uniformity.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (1)
1040-1042: Enhance observability with Micrometer integration.The integration of Micrometer for observability in the
findByBaseIdBranchNameAndApplicationModemethod is a positive change. It allows for better monitoring and performance tracking of database fetch operations. Ensure that the observability tags and metrics are correctly configured and that they provide meaningful insights into the application's performance.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java
Additional comments not posted (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (4)
73-74: Addition of Redis-related static imports.The addition of Redis-related static imports (
APPLICATION_ID_FETCH_REDIS_SPANandAPPLICATION_ID_UPDATE_REDIS_SPAN) suggests that Redis is being used for caching operations. This aligns with the PR's objective to optimize performance using caching mechanisms.
88-88: Introduction of cache name constant.The introduction of
VIEW_MODE_DEFAULT_PAGE_ID_TO_APP_ID_CACHE_NAMEas a constant for the cache name is a good practice. It centralizes the configuration of cache names, making the codebase easier to maintain and modify if needed.
106-106: Addition ofCacheManagerdependency.The addition of
CacheManageras a dependency is crucial for managing cache operations. This change is necessary for implementing the caching logic described in the PR summary.
206-262: Complex caching logic ingetConsolidatedInfoForPageLoad.The method
getConsolidatedInfoForPageLoadhas been significantly modified to include complex caching logic. This includes:
- Checking for cache hits and misses.
- Fetching data from the cache or querying the service as needed.
- Updating the cache with new data if a cache miss occurs.
This logic is central to achieving the performance improvements described in the PR. However, it's important to ensure that the cache is properly invalidated when data changes to avoid serving stale data.
Verification successful
Cache Invalidation Logic Verified
The caching logic in the
getConsolidatedInfoForPageLoadmethod appears to be properly managed, with mechanisms in place to invalidate cache entries when necessary. This is supported by the presence of cache eviction logic in relevant parts of the codebase, such asApplicationPageServiceCEImpl.javaandCacheAspect.java. Additionally, cache invalidation is being tested inFeatureFlagServiceCETest.java, indicating a comprehensive approach to cache management.
- Relevant Files:
ApplicationPageServiceCEImpl.java: Contains logic to evict cache entries related to page IDs.CacheAspect.java: Includes general cache eviction logic.FeatureFlagServiceCETest.java: Tests cache invalidation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper cache invalidation logic. # Test: Search for cache invalidation logic. Expect: Proper handling of cache invalidation. rg --type java -A 5 $'cacheManager.*put'Length of output: 1935
Script:
#!/bin/bash # Description: Verify proper cache invalidation logic by searching for cache eviction or invalidation methods. # Test: Search for cache invalidation logic. Expect: Proper handling of cache invalidation. rg --type java -A 5 $'cacheManager.*(evict|invalidate|remove)'Length of output: 3103
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ApplicationSpanCE.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ApplicationSpanCE.java
...er/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10737886692. |
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java
Outdated
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-36118.dp.appsmith.com |
|
Adding @abhvsn also for a quick review |
subrata71
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/CacheAspect.java (1)
Line range hint
206-259: Refactor to improve readability and efficiency in cache eviction logic.The changes introduced in the
cacheEvictmethod to handle multiple keys are a significant improvement, allowing for dynamic key evaluation and batch processing of cache evictions. However, there are a few areas where the code could be further optimized and made more readable:
Duplication of
StandardEvaluationContextandExpressionParser: The creation ofStandardEvaluationContextandExpressionParserinstances inside the loop (lines 223-224) is inefficient. These should be instantiated once outside the loop to avoid unnecessary object creation during each iteration.Handling of
Listkeys: The current implementation checks if the evaluated key is aListand then processes each key individually (lines 241-247). This is correct, but the loop can be simplified using Java Streams or Reactor'sFluxfor more concise and functional-style code.Error handling: There is no specific error handling for the case where the key evaluation fails or returns an invalid key. Adding error handling could prevent runtime exceptions and provide better stability.
Logging: Adding logging before and after the eviction process can help in debugging and monitoring the cache eviction process.
Here are the suggested changes:
- ExpressionParser parser = new SpelExpressionParser(); - StandardEvaluationContext context = new StandardEvaluationContext(); + // Move these initializations outside the loop to improve performance + ExpressionParser parser = this.EXPRESSION_PARSER; // Reuse the existing parser + StandardEvaluationContext context = new StandardEvaluationContext(); for (int i = 0; i < joinPoint.getArgs().length; i++) { context.setVariable(parameterNames[i], joinPoint.getArgs()[i]); } for (String keyExpression : keys) { try { Expression expression = parser.parseExpression(keyExpression); Object keyObj = expression.getValue(context); if (keyObj instanceof List) { List<?> keyList = (List<?>) keyObj; Flux.fromIterable(keyList) .filter(Objects::nonNull) .map(Object::toString) .flatMap(key -> cacheManager.evict(cacheName, key)) .collectList() .subscribe(); // Handle the results appropriately } else { if (keyObj != null) { cacheManager.evict(cacheName, keyObj.toString()).subscribe(); } } } catch (Exception e) { log.error("Failed to process cache eviction for key: {}", keyExpression, e); } }These changes will enhance the performance by reducing object creation, improve error handling, and make the code more readable and maintainable.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (6 hunks)
- app/server/reactive-caching/src/main/java/com/appsmith/caching/annotations/CacheEvict.java (1 hunks)
- app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/CacheAspect.java (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java
Additional context used
Learnings (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (1)
Learnt from: NilanshBansal PR: appsmithorg/appsmith#33641 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25 Timestamp: 2024-05-26T02:49:06.780Z Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
Learnt from: NilanshBansal PR: appsmithorg/appsmith#33641 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25 Timestamp: 2024-05-26T02:49:06.780Z Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (2)
28-28: Well-defined method for fetching base application ID.The method
fetchBaseApplicationIdis clearly defined and aligns well with the interface's purpose. It uses reactive programming principles effectively, which is suitable for the asynchronous operations typical in web applications.
30-30: Effective cache eviction method.The method
evictCachedBasePageIdsis aptly designed for its purpose. It handles the eviction of cached entries effectively, using a reactive approach that is consistent with modern caching strategies.app/server/reactive-caching/src/main/java/com/appsmith/caching/annotations/CacheEvict.java (1)
31-31: Enhanced flexibility with the newkeysattribute.The addition of the
keysattribute to theCacheEvictannotation significantly enhances its flexibility, allowing for bulk eviction operations. The documentation is clear and helpful, ensuring that developers understand its usage.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (3)
46-46: Good use of constants for cache names.Defining the constant
CACHE_DEFAULT_PAGE_ID_TO_DEFAULT_APPLICATION_IDis a good practice. It ensures consistency and reduces the likelihood of errors in cache naming across different parts of the application.
206-210: Correct implementation of caching infetchBaseApplicationId.The method
fetchBaseApplicationIdis implemented correctly with appropriate use of caching. The conditional return based onbaseApplicationIdpresence is a sensible approach, ensuring that unnecessary database calls are avoided.
212-216: Effective implementation of bulk cache eviction.The method
evictCachedBasePageIdseffectively uses the newkeysattribute in theCacheEvictannotation to perform bulk cache eviction. The implementation is straightforward and aligns well with the caching strategy.
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
Show resolved
Hide resolved
...erver/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
subrata71
added
ok-to-test
…36118) ## Description This PR aims to enhance the performance of the pages span in consolidated-API view mode. It does so by brining the following changes - **Add Projection:** Implement projections to enhance performance for `appsmith.consolidated-api.view.pages.getpage`. - **Implement Caching:** Introduce caching to eliminate the `getpage` query, thereby optimizing `appsmith.consolidated-api.view.application_id` and related spans. We will implement caching to store `defaultApplicationId` information in memory. This cache will reduce the need for the time-consuming `getpage` query. On a cache miss, the system will first fetch the page and then the branched application. If the information is present in the cache, it will directly retrieve the branched application, leveraging the cached data for improved efficiency. With these changes, the pages API is showing improvements. A few screenshots from local environment:     Fixes appsmithorg#36102 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10778438378> > Commit: 25e71b5 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10778438378&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Mon, 09 Sep 2024 18:03:52 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced page retrieval capabilities with additional parameters for improved data handling. - Introduced a caching mechanism for optimized retrieval of application IDs in view mode. - Added support for bulk cache eviction through the `@CacheEvict` annotation. - **Bug Fixes** - Adjusted method signatures in tests to align with updated service methods, ensuring proper functionality. - **Documentation** - Updated test cases to reflect changes in method parameters for clarity and accuracy. - **Chores** - Refactored method calls across various services and tests to incorporate new parameters and improve overall performance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
4 participants
Description
This PR aims to enhance the performance of the pages span in consolidated-API view mode. It does so by brining the following changes
Add Projection: Implement projections to enhance performance for
appsmith.consolidated-api.view.pages.getpage.Implement Caching: Introduce caching to eliminate the
getpagequery, thereby optimizingappsmith.consolidated-api.view.application_idand related spans.We will implement caching to store
defaultApplicationIdinformation in memory. This cache will reduce the need for the time-consuminggetpagequery. On a cache miss, the system will first fetch the page and then the branched application. If the information is present in the cache, it will directly retrieve the branched application, leveraging the cached data for improved efficiency.With these changes, the pages API is showing improvements. A few screenshots from local environment:
Fixes #36102
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10778438378
Commit: 25e71b5
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 09 Sep 2024 18:03:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
@CacheEvictannotation.Bug Fixes
Documentation
Chores