chore: stability pr for changes#35911
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitExecutorCEImpl
participant CommonGitFileUtilsCE
participant GitRepo
User->>GitExecutorCEImpl: Request to push application
GitExecutorCEImpl->>GitRepo: Initiates push operation
GitRepo-->>GitExecutorCEImpl: Acknowledges push
GitExecutorCEImpl->>CommonGitFileUtilsCE: Calls saveArtifactToLocalRepo
CommonGitFileUtilsCE->>GitRepo: Saves application to local repo
GitRepo-->>CommonGitFileUtilsCE: Confirms save
CommonGitFileUtilsCE-->>GitExecutorCEImpl: Returns path
GitExecutorCEImpl-->>User: Confirms application push
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
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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java
Additional comments not posted (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
Line range hint
37-99: Great job on improving responsiveness!The addition of
Schedulers.boundedElastic()to offload the potentially blocking operation to a separate thread is a good practice in reactive programming. This will help in preventing the main execution flow from being blocked.The code changes are approved. However, ensure to monitor the impact of this change on the overall application performance.
| // this subscribeOn on is required because Mono.using | ||
| // is not deferring the execution of push and for that reason it runs on the | ||
| // lettuce-nioEventLoop thread instead of boundedElastic | ||
| .subscribeOn(scheduler); |
There was a problem hiding this comment.
Should the inner callable be subscribed instead?
There was a problem hiding this comment.
It's the inner callable only.
| return fileUtils.saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName); | ||
| return fileUtils | ||
| .saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName) | ||
| .subscribeOn(Schedulers.boundedElastic()); |
There was a problem hiding this comment.
Should this be published on immediate to return back to the owning thread pool?
There was a problem hiding this comment.
Since this flow is followed by other blocking calls in all 5 cases, we are good with not publishing this on.
However, I would keep this point handy in order to test further.
## Description - Changes to put blocking calls on the bounded elastic thread. - This Pr has changes to test. - commit - Status - branch - delete branch - List branch - checkout - checkout remote - merge status - merge Fixes #`Issue Number` _or_ Fixes `Issue URL` ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved performance and responsiveness when saving applications to Git repositories by optimizing the execution flow. - Enhanced clarity and control in the application push process to Git, ensuring better maintainability. - **Bug Fixes** - Addressed potential threading issues by ensuring operations run on the appropriate scheduler. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Fixes #
Issue Numberor
Fixes
Issue URLAutomation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10628395436
Commit: af76a97
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 30 Aug 2024 07:29:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes