Skip to content

chore: stability pr for changes #35911

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

Merged
sondermanish merged 1 commit into from
Aug 30, 2024
Merged

chore: stability pr for changes #35911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,8 @@ public Mono<String> pushApplication(
// 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 "
Expand Down Expand Up @@ -265,9 +263,12 @@ public Mono<String> pushApplication(
.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);
Copy link
Contributor

@nidhi-nair nidhi-nair Aug 29, 2024

Choose a reason for hiding this comment

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

Should the inner callable be subscribed instead?

Copy link
Contributor Author

@sondermanish sondermanish Aug 29, 2024

Choose a reason for hiding this comment

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

It's the inner callable only.

});
}

/** Clone the repo to the file path : container-volume/orgId/defaultAppId/repo/<Data>
Expand Down
5 changes: 4 additions & 1 deletion ...er/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
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;
Expand Down Expand Up @@ -93,7 +94,9 @@ public Mono<Path> saveArtifactToLocalRepo(

// Save application to git repo
try {
return fileUtils.saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName);
return fileUtils
.saveApplicationToGitRepo(baseRepoSuffix, artifactGitReference, branchName)
.subscribeOn(Schedulers.boundedElastic());
Copy link
Contributor

@nidhi-nair nidhi-nair Aug 29, 2024

Choose a reason for hiding this comment

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

Should this be published on immediate to return back to the owning thread pool?

Copy link
Contributor Author

@sondermanish sondermanish Aug 30, 2024

Choose a reason for hiding this comment

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

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.

} catch (IOException | GitAPIException e) {
log.error("Error occurred while saving files to local git repo: ", e);
throw Exceptions.propagate(e);
Expand Down