Skip deploy 611#16123
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to skip application deployment if an identical version and configuration are already present, and enables optimistic locking for Spanner transactions. Feedback focuses on refining the deployment check logic to handle version ranges and version mismatches correctly, using semantic JSON comparison instead of string matching, and adjusting log levels. Additionally, there is a concern regarding the global impact of enabling optimistic locking on Spanner transaction retry rates.
| * configuration. | ||
| */ | ||
| public boolean isAppAlreadyDeployed(ApplicationId appId, AppRequest<?> appRequest) throws Exception { | ||
| ApplicationMeta appMeta = store.getLatest(appId.getAppReference()); |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| try { | ||
| if (applicationLifecycleService.isAppAlreadyDeployed(appId, appRequest)) { | ||
| LOG.warn("Application {} is already deployed",appId ); |
There was a problem hiding this comment.
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.
| LOG.warn("Application {} is already deployed",appId ); | |
| LOG.info("Application {} is already deployed", appId); |
| * Checks if the application already exists with the exact same artifact and | ||
| * configuration. | ||
| */ | ||
| public boolean isAppAlreadyDeployed(ApplicationId appId, AppRequest<?> appRequest) throws Exception { |
There was a problem hiding this comment.
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.
| String normRequestedConfig = requestedConfigStr == null ? "" : requestedConfigStr.trim(); | ||
| String normCurrentConfig = currentConfigStr == null ? "" : currentConfigStr.trim(); | ||
|
|
||
| if (!normRequestedConfig.equals(normCurrentConfig)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
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.
| public void run(TxRunnable runnable) throws TransactionException { | ||
| try { | ||
| admin.getDatabaseClient().readWriteTransaction().allowNestedTransaction().run(context -> { | ||
| admin.getDatabaseClient().readWriteTransaction(Options.optimisticLock()).allowNestedTransaction().run(context -> { |
There was a problem hiding this comment.
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.
1 participant
No description provided.