Skip to content

Skip deploy 611 #16123

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

Draft
sahusanket wants to merge 4 commits into
base: release/6.11
Choose a base branch
from
+57 −1
Draft

Skip deploy 611 #16123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69c38c2
Check config and aritfact and skip
sahusanket May 5, 2026
b5aefb3
optimistic lock
sahusanket May 6, 2026
de34764
support for version range
sahusanket May 6, 2026
34b452a
comment
sahusanket May 11, 2026
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
8 changes: 8 additions & 0 deletions ...p-fabric/src/main/java/io/cdap/cdap/gateway/handlers/AbstractAppLifecycleHttpHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,14 @@ protected void onFinish(HttpResponder responder, File uploadedFile) {
AppRequest<?> appRequest = DECODE_GSON.fromJson(fileReader, AppRequest.class);

try {
if (applicationLifecycleService.isAppAlreadyDeployed(appId, appRequest)) {
LOG.warn("Application {} is already deployed",appId );
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

medium

The use of LOG.warn is inappropriate here as skipping a deployment because it is already up-to-date is a normal, expected condition. LOG.info or LOG.debug would be more suitable. Additionally, there is a missing space after the comma in the log arguments.

Suggested change
LOG.warn("Application {} is already deployed",appId );
LOG.info("Application {} is already deployed", appId);

io.cdap.cdap.proto.ApplicationDetail existingApp =
applicationLifecycleService.getLatestAppDetail(appId.getAppReference());
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(new io.cdap.cdap.proto.ApplicationRecord(existingApp)));
return;
}

ApplicationWithPrograms app = applicationLifecycleService.deployApp(appId, appRequest,
null, createProgramTerminator(), skipMarkingLatest);
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(getApplicationRecord(app)));
Expand Down
46 changes: 46 additions & 0 deletions ...-fabric/src/main/java/io/cdap/cdap/internal/app/services/ApplicationLifecycleService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,52 @@ private ApplicationId updateApplicationByArtifact(ApplicationId appId,
allowedArtifactScopes, allowSnapshot, ownerAdmin.getOwner(appId), appSpec);
}

/**
* Checks if the application already exists with the exact same artifact and
* configuration.
*/
public boolean isAppAlreadyDeployed(ApplicationId appId, AppRequest<?> appRequest) throws Exception {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

medium

The method signature throws Exception, which is too broad. It is better to catch specific exceptions internally or declare more specific exceptions (e.g., IOException) to improve error handling and maintainability.

ApplicationMeta appMeta = store.getLatest(appId.getAppReference());
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

high

The logic currently checks the latest version of the application using appId.getAppReference(). If the appId provided in the request specifies a different version than the latest one currently deployed, the deployment should likely proceed to create that new version, even if the artifact and configuration are identical. Consider verifying that the version in appId matches the version of the existing application found in appMeta.

if (appMeta == null || appMeta.getSpec() == null) {
return false;
}

ArtifactSummary requestedArtifact = appRequest.getArtifact();
if (requestedArtifact == null) {
return false;
}

ArtifactId currentArtifactId = appMeta.getSpec().getArtifactId();
if (!currentArtifactId.getName().equals(requestedArtifact.getName()) ||
!currentArtifactId.getScope().equals(requestedArtifact.getScope())) {
return false;
}

try {
io.cdap.cdap.api.artifact.ArtifactVersionRange range = io.cdap.cdap.api.artifact.ArtifactVersionRange
.parse(requestedArtifact.getVersion());
if (!range.versionIsInRange(currentArtifactId.getVersion())) {
return false;
}
} catch (Exception e) {
return false;
}
Comment on lines +765 to +773
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

high

Using ArtifactVersionRange.versionIsInRange to determine if an app is already deployed can be problematic when a version range is requested. If a newer artifact version that also satisfies the range is available in the repository, a standard deployment would typically upgrade the application. By skipping the deployment here, you might prevent such updates. It is safer to skip only if the requested version is a specific version and it matches the currently deployed version exactly.


Object config = appRequest.getConfig();
String requestedConfigStr = config == null ? null
: config instanceof String ? (String) config : GSON.toJson(config);
String currentConfigStr = appMeta.getSpec().getConfiguration();

String normRequestedConfig = requestedConfigStr == null ? "" : requestedConfigStr.trim();
String normCurrentConfig = currentConfigStr == null ? "" : currentConfigStr.trim();

if (!normRequestedConfig.equals(normCurrentConfig)) {
return false;
}
Comment on lines +780 to +785
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

medium

Comparing JSON configurations as trimmed strings is fragile and can lead to false negatives if the JSON key ordering or whitespace differs, even when the configurations are semantically identical. A more robust approach is to compare the configurations as JsonElement objects using GSON's equality check.


return true;
}

/**
* Updates an application config by applying given update actions. The app should know how to
* apply these actions to its config.
Expand Down
4 changes: 3 additions & 1 deletion ...rage-ext-spanner/src/main/java/io/cdap/cdap/storage/spanner/SpannerTransactionRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.cdap.cdap.storage.spanner;

import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.Options;
import com.google.cloud.spanner.SpannerException;
import io.cdap.cdap.spi.data.transaction.TransactionException;
import io.cdap.cdap.spi.data.transaction.TransactionRunner;
Expand All @@ -36,7 +37,8 @@ public SpannerTransactionRunner(SpannerStructuredTableAdmin admin) {
@Override
public void run(TxRunnable runnable) throws TransactionException {
try {
admin.getDatabaseClient().readWriteTransaction().allowNestedTransaction().run(context -> {
// enforce lock
admin.getDatabaseClient().readWriteTransaction(Options.optimisticLock()).allowNestedTransaction().run(context -> {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

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

medium

Enabling Options.optimisticLock() globally for all Spanner transactions is a significant change. While it can improve performance by reducing lock contention during the read phase, it may also increase the rate of transaction aborts and retries in high-contention scenarios. Please ensure this change is intended to be global and has been verified against various CDAP workloads, as it seems unrelated to the 'Skip deploy' logic.

runnable.run(new SpannerStructuredTableContext(context, admin));
return null;
});
Expand Down