From 4b89c1c7a75a6b36de6e1a8a20af29f73e6574e1 Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Fri, 30 Aug 2024 12:59:15 +0530 Subject: [PATCH] chore: stability pr for changes (#35911) ## 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" ### :mag: Cypress test results > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## 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. --- .../git/service/ce/GitExecutorCEImpl.java | 15 ++++++++------- .../server/helpers/ce/CommonGitFileUtilsCE.java | 5 ++++- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java index d0c71d7028..bf0f3ab349 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java @@ -224,10 +224,8 @@ public class GitExecutorCEImpl implements GitExecutor { // open the repo Path baseRepoPath = createRepoPath(repoSuffix); - return gitConfig - .getIsAtomicPushAllowed() - .flatMap(isAtomicPushAllowed -> { - return Mono.using( + return gitConfig.getIsAtomicPushAllowed().flatMap(isAtomicPushAllowed -> { + return Mono.using( () -> Git.open(baseRepoPath.toFile()), git -> Mono.fromCallable(() -> { log.debug(Thread.currentThread().getName() + ": pushing changes to remote " @@ -265,9 +263,12 @@ public class GitExecutorCEImpl implements GitExecutor { .timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS)) .name(GitSpan.FS_PUSH) .tap(Micrometer.observation(observationRegistry)), - Git::close); - }) - .subscribeOn(scheduler); + Git::close) + // 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); + }); } /** Clone the repo to the file path : container-volume/orgId/defaultAppId/repo/ diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java index a648871bb1..5cfd3c2a2b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java @@ -34,6 +34,7 @@ import org.springframework.context.annotation.Import; import org.springframework.stereotype.Component; import reactor.core.Exceptions; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Schedulers; import java.io.IOException; import java.nio.file.Path; @@ -93,7 +94,9 @@ public class CommonGitFileUtilsCE { // Save application to git repo try { - return fileUtils.saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName); + return fileUtils + .saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName) + .subscribeOn(Schedulers.boundedElastic()); } catch (IOException | GitAPIException e) { log.error("Error occurred while saving files to local git repo: ", e); throw Exceptions.propagate(e);