Skip to content

chore: package refactors and left over changes for git autocommit pr #33768

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 2 commits into from
May 29, 2024
Merged

chore: package refactors and left over changes for git autocommit pr #33768

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8328f61
added leftover changes
sondermanish May 27, 2024
5130215
added refactors
sondermanish May 27, 2024
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
10 changes: 6 additions & 4 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,19 @@ public class GitRedisUtils {

private final RedisUtils redisUtils;

public Mono<Boolean> addFileLock(String defaultApplicationId) {
public Mono<Boolean> addFileLock(String defaultApplicationId, Boolean isRetryAllowed) {
long numberOfRetries = Boolean.TRUE.equals(isRetryAllowed) ? MAX_RETRIES : 0L;

return redisUtils
.addFileLock(defaultApplicationId)
.retryWhen(Retry.fixedDelay(MAX_RETRIES, RETRY_DELAY)
.retryWhen(Retry.fixedDelay(numberOfRetries, RETRY_DELAY)
.onRetryExhaustedThrow((retryBackoffSpec, retrySignal) -> {
throw new AppsmithException(AppsmithError.GIT_FILE_IN_USE);
}));
}

public Mono<Boolean> addFileLockWithoutRetry(String defaultApplicationId) {
return redisUtils.addFileLock(defaultApplicationId);
public Mono<Boolean> addFileLock(String defaultApplicationId) {
return addFileLock(defaultApplicationId, Boolean.TRUE);
}

public Mono<Boolean> releaseFileLock(String defaultApplicationId) {
Expand Down
2 changes: 1 addition & 1 deletion .../main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public Mono<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactM
.defaultIfEmpty(FALSE)
.cache();

return Mono.defer(() -> gitRedisUtils.addFileLockWithoutRetry(defaultApplicationId))
return Mono.defer(() -> gitRedisUtils.addFileLock(defaultApplicationId, FALSE))

@coderabbitai coderabbitai Bot May 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The search results indicate that several instances of addFileLock across different files do not include the isRetryAllowed parameter. These instances need to be updated to use the new method signature.

Files and Instances to Update:

  • AutoCommitEventHandlerCEImpl.java

    • private Mono<Boolean> addFileLock(String defaultApplicationId)
    • .addFileLock(defaultApplicationId)
    • return addFileLock(autoCommitEvent.getApplicationId())
    • return addFileLock(defaultApplicationId)

  • CommonGitServiceCEImpl.java

    • return Mono.defer(() -> addFileLock(defaultArtifactId))
    • private Mono<Boolean> addFileLock(String defaultArtifactId)
    • .addFileLock(defaultArtifactId)
    • Mono<Boolean> fileLockMono = Mono.defer(() -> addFileLock(defaultArtifactId))
    • addFileLockMono = addFileLock(defaultArtifactId)
    • return addFileLock(tuple3.getT1().getDefaultArtifactId()).then(Mono.just(tuple3))
    • return addFileLock(gitData.getDefaultArtifactId()).then(Mono.just(artifact))
    • return addFileLock(artifact.getGitArtifactMetadata().getDefaultArtifactId())
    • sourceAritfactMono = addFileLock(defaultArtifactId)
    • Mono<? extends Artifact> checkoutRemoteBranchMono = addFileLock(defaultArtifactId)
    • return addFileLock(srcBranchGitData.getDefaultArtifactId())
    • return addFileLock(gitMetadata.getDefaultArtifactId())
    • .flatMap(defaultArtifact -> addFileLock(defaultArtifactId).map(status -> defaultArtifact))
    • .flatMap(branchedArtifact -> addFileLock(defaultArtifactId).thenReturn(branchedArtifact))
    • return addFileLock(gitData.getDefaultArtifactId()).then(Mono.just(defaultArtifact))
    • return addFileLock(defaultArtifactId)
    • return addFileLock(metadata.getDefaultArtifactId())
    • return addFileLock(defaultArtifact.getId())

  • RedisUtils.java

    • public Mono<Boolean> addFileLock(String key)

  • AutoCommitEligibilityHelperTest.java

    • Mockito.when(redisUtils.addFileLock(DEFAULT_APPLICATION_ID)).thenReturn(Mono.just(Boolean.TRUE))

Action Required:

Update the above instances to include the isRetryAllowed parameter in the method calls to addFileLock.

Analysis chain

Update to use the new addFileLock method signature with retry control.

Verify all instances of addFileLock are updated to use the new method signature with isRetryAllowed.

Verify specific instances of addFileLock without the isRetryAllowed parameter.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify integration of the new `addFileLock` method in the system.

# Test: Search for other usages of `addFileLock` to ensure they are updated.
rg --type java $'addFileLock'

Length of output: 10755

Script:

#!/bin/bash
# Description: Verify all instances of `addFileLock` are updated to use the new method signature with `isRetryAllowed`.

# Test: Search for method definitions and usages of `addFileLock` to ensure they include the `isRetryAllowed` parameter.
rg --type java 'addFileLock\('

Length of output: 8737

Script:

#!/bin/bash
# Description: Extract lines from specific files to verify if `addFileLock` needs updating.

# Files to check
files=(
  "app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCEImpl.java"
  "app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java"
  "app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java"
  "app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java"
)

# Extract lines containing `addFileLock` without `isRetryAllowed`
for file in "${files[@]}"; do
  echo "Checking $file"
  rg 'addFileLock\([^,]*\)' "$file"
done

Length of output: 3826

.then(Mono.defer(() -> isServerMigrationRequiredMonoCached))
.then(Mono.defer(() -> gitRedisUtils.releaseFileLock(defaultApplicationId)))
.then(Mono.defer(() -> isServerMigrationRequiredMonoCached))
Expand Down