Apply review feedback

Author: gmarouli

x-pack/plugin/downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

@@ -174,10 +174,11 @@ private void downsampleAndAssert(String dataStreamName, String mapping, Supplier
     }
 
     private String generateForceMergeMetadata() {
-        return switch (randomIntBetween(0, 3)) {
+        return switch (randomIntBetween(0, 4)) {
             case 0 -> "\"_meta\": { \"downsample.forcemerge.enabled\": false},";
             case 1 -> "\"_meta\": { \"downsample.forcemerge.enabled\": true},";
             case 2 -> "\"_meta\": { \"downsample.forcemerge.enabled\": 4},";
+            case 3 -> "\"_meta\": { \"downsample.forcemerge.enabled\": null},";
             default -> "";
         };
     }

x-pack/plugin/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java

@@ -301,16 +301,7 @@ protected void masterOperation(
         final TaskId parentTask = new TaskId(clusterService.localNode().getId(), task.getId());
         // Short circuit if target index has been downsampled:
         final String downsampleIndexName = request.getTargetIndex();
-        if (canShortCircuit(
-            sourceIndexName,
-            downsampleIndexName,
-            parentTask,
-            request.masterNodeTimeout(),
-            request.getWaitTimeout(),
-            startTime,
-            projectMetadata,
-            listener
-        )) {
+        if (canShortCircuit(downsampleIndexName, parentTask, request.getWaitTimeout(), startTime, true, projectMetadata, listener)) {
             logger.info("Skipping downsampling, because a previous execution already completed downsampling");
             return;
         }
@@ -479,73 +470,6 @@ private boolean isForceMergeEnabled(Map<String, Object> sourceIndexMappings) {
         return true;
     }
 
-    /**
-     * Shortcircuit when another downsample api invocation already completed successfully.
-     */
-    private boolean canShortCircuit(
-        String sourceIndexName,
-        String targetIndexName,
-        TaskId parentTask,
-        TimeValue masterNodeTimeout,
-        TimeValue waitTimeout,
-        long startTime,
-        ProjectMetadata projectMetadata,
-        ActionListener<AcknowledgedResponse> listener
-    ) {
-        IndexMetadata targetIndexMetadata = projectMetadata.index(targetIndexName);
-        if (targetIndexMetadata == null) {
-            return false;
-        }
-
-        var downsampleStatus = IndexMetadata.INDEX_DOWNSAMPLE_STATUS.get(targetIndexMetadata.getSettings());
-        if (downsampleStatus == DownsampleTaskStatus.UNKNOWN) {
-            // This isn't a downsample index, so fail:
-            listener.onFailure(new ResourceAlreadyExistsException(targetIndexMetadata.getIndex()));
-            return true;
-        } else if (downsampleStatus == DownsampleTaskStatus.SUCCESS) {
-            listener.onResponse(AcknowledgedResponse.TRUE);
-            return true;
-        }
-        // In case the write block has been set on the target index means that the shard level downsampling itself was successful,
-        // but the previous invocation failed later performing settings update, refresh or force merge.
-        // The write block is used a signal to resume from the refresh part of the downsample api invocation.
-        if (targetIndexMetadata.getSettings().get(IndexMetadata.SETTING_BLOCKS_WRITE) != null) {
-            // 1. Extract source index mappings
-            final GetMappingsRequest getMappingsRequest = new GetMappingsRequest(masterNodeTimeout).indices(sourceIndexName);
-            getMappingsRequest.setParentTask(parentTask);
-            client.admin().indices().getMappings(getMappingsRequest, listener.delegateFailureAndWrap((delegate, getMappingsResponse) -> {
-                final Map<String, Object> sourceIndexMappings = getMappingsResponse.mappings()
-                    .entrySet()
-                    .stream()
-                    .filter(entry -> sourceIndexName.equals(entry.getKey()))
-                    .findFirst()
-                    .map(mappingMetadata -> mappingMetadata.getValue().sourceAsMap())
-                    .orElseThrow(
-                        () -> new IllegalArgumentException("No mapping found for downsample source index [" + sourceIndexName + "]")
-                    );
-                var forceMergeEnabled = isForceMergeEnabled(sourceIndexMappings);
-                var refreshRequest = new RefreshRequest(targetIndexMetadata.getIndex().getName());
-                refreshRequest.setParentTask(parentTask);
-                client.admin()
-                    .indices()
-                    .refresh(
-                        refreshRequest,
-                        new RefreshDownsampleIndexActionListener(
-                            projectMetadata.id(),
-                            delegate,
-                            parentTask,
-                            targetIndexMetadata.getIndex().getName(),
-                            waitTimeout,
-                            startTime,
-                            forceMergeEnabled
-                        )
-                    );
-            }));
-            return true;
-        }
-        return false;
-    }
-
     /**
      * Shortcircuit when another downsample api invocation already completed successfully.
      */

x-pack/plugin/downsample/src/test/java/org/elasticsearch/xpack/downsample/TransportDownsampleActionTests.java

@@ -279,21 +279,12 @@ private void downsample(String mapping) throws IOException {
     }
 
     public void testDownsamplingForceMergeWithShortCircuitAfterCreation() {
-        String mapping = switch (randomIntBetween(0, 2)) {
+        String mapping = switch (randomIntBetween(0, 3)) {
             case 0 -> NO_METADATA_MAPPING;
             case 1 -> OTHER_METADATA_MAPPING;
-            default -> FORCE_MERGE_ENABLED_MAPPING;
+            case 2 -> FORCE_MERGE_ENABLED_MAPPING;
+            default -> FORCE_MERGE_DISABLED_MAPPING;
         };
-        downsampleSkipsForceMergeWithShortCircuitAfterCreation(mapping);
-        verify(indicesAdminClient).forceMerge(any(), any());
-    }
-
-    public void testDownsamplingSkipsForceMergeWithShortCircuitAfterCreation() {
-        downsampleSkipsForceMergeWithShortCircuitAfterCreation(FORCE_MERGE_DISABLED_MAPPING);
-        verify(indicesAdminClient, never()).forceMerge(any(), any());
-    }
-
-    public void downsampleSkipsForceMergeWithShortCircuitAfterCreation(String mapping) {
         mockGetMapping(mapping);
 
         var projectMetadata = ProjectMetadata.builder(projectId)
@@ -323,6 +314,7 @@ public void downsampleSkipsForceMergeWithShortCircuitAfterCreation(String mappin
         );
         safeGet(listener);
         verify(downsampleMetrics).recordOperation(anyLong(), eq(DownsampleMetrics.ActionStatus.SUCCESS));
+        verify(indicesAdminClient).forceMerge(any(), any());
     }
 
     public void testDownsamplingForceMergeWithShortCircuitDuringCreation() throws IOException {