Fix all the compile warnings and detekt issues (#603)

* Fix all the compile warnings and detekt issues Signed-off-by: bowenlan-amzn <[email protected]> * Fix time capture is 0 Signed-off-by: bowenlan-amzn <[email protected]> Signed-off-by: bowenlan-amzn <[email protected]>
opensearch-project · Nov 21, 2022 · f4741c2 · f4741c2
1 parent c7df8df
commit f4741c2
Show file tree

Hide file tree

Showing 66 changed files with 238 additions and 181 deletions.
diff --git a/.github/workflows/bwc-test-workflow.yml b/.github/workflows/bwc-test-workflow.yml
@@ -0,0 +1,32 @@
+name: Backward compatibility test workflow
+on:
+  pull_request:
+    branches:
+      - "*"
+  push:
+    branches:
+      - "*"
+
+jobs:
+  test:
+    # This job runs on Linux
+    runs-on: ubuntu-latest
+    steps:
+      # This step uses the setup-java Github action: https://github.com/actions/setup-java
+      - name: Set Up JDK 11
+        uses: actions/setup-java@v1
+        with:
+          java-version: 11
+      # index-management
+      - name: Checkout Branch
+        uses: actions/checkout@v2
+      - name: Run IM Backwards Compatibility Tests
+        run: |
+          echo "Running backwards compatibility tests..."
+          ./gradlew bwcTestSuite
+      - name: Upload failed logs
+        uses: actions/upload-artifact@v2
+        if: failure()
+        with:
+          name: logs
+          path: build/testclusters/indexmanagementBwcCluster*/logs/*
diff --git a/.github/workflows/create-documentation-issue.yml b/.github/workflows/create-documentation-issue.yml
@@ -2,6 +2,7 @@ name: Create Documentation Issue
 on:
   pull_request:
     types:
+      - closed
       - labeled
 env:
   PR_NUMBER: ${{ github.event.number }}

diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml
diff --git a/.github/workflows/links.yml b/.github/workflows/links.yml
@@ -6,7 +6,7 @@ on:
     branches: [ main ]
 
 jobs:
-  linkchecker:
+  check:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2

diff --git a/.github/workflows/multi-node-test-workflow.yml b/.github/workflows/multi-node-test-workflow.yml
@@ -9,9 +9,7 @@ on:
       - "*"
 
 jobs:
-  build:
-    # Job name
-    name: Build Index Management
+  test:
     # This job runs on Linux
     runs-on: ubuntu-latest
     steps:
@@ -31,20 +29,3 @@ jobs:
         with:
           name: logs
           path: build/testclusters/integTest-*/logs/*
-  bwc:
-    name: Run Index Management Backwards Compatibility Tests
-    # This job runs on Linux
-    runs-on: ubuntu-latest
-    steps:
-      # This step uses the setup-java Github action: https://github.com/actions/setup-java
-      - name: Set Up JDK 11
-        uses: actions/setup-java@v1
-        with:
-          java-version: 11
-      # index-management
-      - name: Checkout Branch
-        uses: actions/checkout@v2
-      - name: Run IM Backwards Compatibility Tests
-        run: |
-          echo "Running backwards compatibility tests..."
-          ./gradlew bwcTestSuite
diff --git a/.github/workflows/test-and-build-workflow.yml b/.github/workflows/test-and-build-workflow.yml
@@ -9,8 +9,6 @@ on:
 
 jobs:
   build:
-    # Job name
-    name: Build Index Management
     env:
       BUILD_ARGS: ${{ matrix.os_build_args }}
       WORKING_DIR: ${{ matrix.working_directory }}.

diff --git a/build.gradle b/build.gradle
@@ -137,20 +137,24 @@ task ktlint(type: JavaExec, group: "verification") {
     classpath = configurations.ktlint
     args "src/**/*.kt", "spi/src/main/**/*.kt"
 }
-
 check.dependsOn ktlint
 
 task ktlintFormat(type: JavaExec, group: "formatting") {
     description = "Fix Kotlin code style deviations."
     main = "com.pinterest.ktlint.Main"
     classpath = configurations.ktlint
     args "-F", "src/**/*.kt", "spi/src/main/**/*.kt"
+    // https://github.com/pinterest/ktlint/issues/1391
+    jvmArgs "--add-opens=java.base/java.lang=ALL-UNNAMED"
 }
 
 detekt {
     config = files("detekt.yml")
     buildUponDefaultConfig = true
 }
+// When writing detekt Gradle first finds the extension with this name,
+// but with a string it should look for a task with that name instead
+check.dependsOn "detekt"
 
 configurations.testImplementation {
     exclude module: "securemock"
@@ -656,7 +660,14 @@ run {
     }
 }
 
-compileKotlin { kotlinOptions.freeCompilerArgs = ['-Xjsr305=strict'] }
+compileKotlin {
+    kotlinOptions.freeCompilerArgs = ['-Xjsr305=strict']
+    kotlinOptions.allWarningsAsErrors = true
+}
+
+compileTestKotlin {
+    kotlinOptions.allWarningsAsErrors = true
+}
 
 apply from: 'build-tools/pkgbuild.gradle'
 

diff --git a/detekt.yml b/detekt.yml
@@ -1,6 +1,5 @@
-# TODO: Remove this before initial release, only for developmental purposes
 build:
-  maxIssues: 20
+  maxIssues: 0
 
 exceptions:
   TooGenericExceptionCaught:
@@ -14,6 +13,8 @@ style:
   MaxLineLength:
     maxLineLength: 150
     excludes: ['**/test/**']
+  FunctionOnlyReturningConstant:
+    active: false
 
 complexity:
   LargeClass:

diff --git a/...ain/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexCoordinator.kt b/...ain/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexCoordinator.kt
@@ -160,8 +160,10 @@ class ManagedIndexCoordinator(
         }
         clusterService.clusterSettings.addSettingsUpdateConsumer(METADATA_SERVICE_STATUS) {
             metadataServiceEnabled = it == 0
-            if (!metadataServiceEnabled) scheduledMoveMetadata?.cancel()
-            else initMoveMetadata()
+            if (!metadataServiceEnabled) {
+                logger.info("Canceling metadata moving job because of cluster setting update.")
+                scheduledMoveMetadata?.cancel()
+            } else initMoveMetadata()
         }
         clusterService.clusterSettings.addSettingsUpdateConsumer(TEMPLATE_MIGRATION_CONTROL) {
             templateMigrationEnabled = it >= 0L
@@ -202,8 +204,8 @@ class ManagedIndexCoordinator(
         // Instead of using a LocalNodeMasterListener to track cluster manager changes, this service will
         // track them here to avoid conditions where cluster manager listener events run after other
         // listeners that depend on what happened in the cluster manager listener
-        if (this.isClusterManager != event.localNodeMaster()) {
-            this.isClusterManager = event.localNodeMaster()
+        if (this.isClusterManager != event.localNodeClusterManager()) {
+            this.isClusterManager = event.localNodeClusterManager()
             if (this.isClusterManager) {
                 onClusterManager()
             } else {
@@ -215,7 +217,7 @@ class ManagedIndexCoordinator(
 
         if (event.isNewCluster) return
 
-        if (!event.localNodeMaster()) return
+        if (!event.localNodeClusterManager()) return
 
         if (!event.metadataChanged()) return
 
@@ -380,7 +382,7 @@ class ManagedIndexCoordinator(
     }
 
     /**
-     * Find a policy that has highest priority ism template with matching index pattern to the index and is created before index creation date. If
+     * Find a policy that has the highest priority ism template with matching index pattern to the index and is created before index creation date. If
      * the policy has user, ensure that the user can manage the index if not find the one that can.
      * */
     private suspend fun findMatchingPolicy(indexName: String, creationDate: Long, policies: List<Policy>): Policy? {
@@ -422,7 +424,7 @@ class ManagedIndexCoordinator(
             try {
                 val request = ManagedIndexRequest().indices(indexName)
                 withClosableContext(IndexManagementSecurityContext("ApplyPolicyOnIndexCreation", settings, threadPool.threadContext, policy.user)) {
-                    val response: AcknowledgedResponse = client.suspendUntil { execute(ManagedIndexAction.INSTANCE, request, it) }
+                    client.suspendUntil<Client, AcknowledgedResponse> { execute(ManagedIndexAction.INSTANCE, request, it) }
                 }
             } catch (e: OpenSearchSecurityException) {
                 logger.debug("Skipping applying policy ${policy.id} on $indexName as the policy user is missing permissions", e)
@@ -473,13 +475,13 @@ class ManagedIndexCoordinator(
         // If ISM is disabled return early
         if (!isIndexStateManagementEnabled()) return
 
-        // Do not setup background sweep if we're not the elected cluster manager node
-        if (!clusterService.state().nodes().isLocalNodeElectedMaster) return
+        // Do not set up background sweep if we're not the elected cluster manager node
+        if (!clusterService.state().nodes().isLocalNodeElectedClusterManager) return
 
         // Cancel existing background sweep
         scheduledFullSweep?.cancel()
 
-        // Setup an anti-entropy/self-healing background sweep, in case we fail to create a ManagedIndexConfig job
+        // Set up an anti-entropy/self-healing background sweep, in case we fail to create a ManagedIndexConfig job
         val scheduledSweep = Runnable {
             val elapsedTime = getFullSweepElapsedTime()
 
@@ -505,7 +507,7 @@ class ManagedIndexCoordinator(
     fun initMoveMetadata() {
         if (!metadataServiceEnabled) return
         if (!isIndexStateManagementEnabled()) return
-        if (!clusterService.state().nodes().isLocalNodeElectedMaster) return
+        if (!clusterService.state().nodes().isLocalNodeElectedClusterManager) return
         scheduledMoveMetadata?.cancel()
 
         if (metadataService.finishFlag) {
@@ -535,7 +537,7 @@ class ManagedIndexCoordinator(
     fun initTemplateMigration(enableSetting: Long) {
         if (!templateMigrationEnabled) return
         if (!isIndexStateManagementEnabled()) return
-        if (!clusterService.state().nodes().isLocalNodeElectedMaster) return
+        if (!clusterService.state().nodes().isLocalNodeElectedClusterManager) return
         scheduledTemplateMigration?.cancel()
 
         // if service has finished, re-enable it
@@ -657,8 +659,7 @@ class ManagedIndexCoordinator(
                 if (scrollIDsToClear.isNotEmpty()) {
                     val clearScrollRequest = ClearScrollRequest()
                     clearScrollRequest.scrollIds(scrollIDsToClear.toList())
-                    val clearScrollResponse: ClearScrollResponse =
-                        client.suspendUntil { execute(ClearScrollAction.INSTANCE, clearScrollRequest, it) }
+                    client.suspendUntil<Client, ClearScrollResponse> { execute(ClearScrollAction.INSTANCE, clearScrollRequest, it) }
                 }
             }
             return managedIndexUuids
@@ -693,7 +694,7 @@ class ManagedIndexCoordinator(
         val mRes: MultiGetResponse = client.suspendUntil { multiGet(mReq, it) }
         val responses = mRes.responses
         if (responses.first().isFailed) {
-            // config index may not initialised yet
+            // config index may not initialise yet
             logger.error("get managed-index failed: ${responses.first().failure.failure}")
             return result
         }

diff --git a/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexRunner.kt b/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexRunner.kt
@@ -387,7 +387,7 @@ object ManagedIndexRunner :
         // If this action is not allowed and the step to be executed is the first step in the action then we will fail
         // as this action has been removed from the AllowList, but if its not the first step we will let it finish as it's already inflight
         if (action?.isAllowed(allowList) == false && step != null && action.isFirstStep(step.name) && action.type != TransitionsAction.name) {
-            val info = mapOf("message" to "Attempted to execute action=${action?.type} which is not allowed.")
+            val info = mapOf("message" to "Attempted to execute action=${action.type} which is not allowed.")
             val updated = updateManagedIndexMetaData(
                 managedIndexMetaData.copy(
                     policyRetryInfo = PolicyRetryInfoMetaData(true, 0), info = info

diff --git a/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/MetadataService.kt b/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/MetadataService.kt
@@ -191,12 +191,12 @@ class MetadataService(
 
             override fun onResponse(response: ClusterUpdateSettingsResponse) {
                 if (!response.isAcknowledged) {
-                    logger.error("Update template migration setting to $status is not acknowledged")
+                    logger.error("Update metadata migration setting to $status is not acknowledged")
                     throw IndexManagementException.wrap(
-                        Exception("Update template migration setting to $status is not acknowledged")
+                        Exception("Update metadata migration setting to $status is not acknowledged")
                     )
                 } else {
-                    logger.info("Successfully update template migration setting to $status")
+                    logger.info("Successfully metadata template migration setting to $status")
                 }
             }
         }

diff --git a/...nsearch/indexmanagement/indexstatemanagement/model/coordinator/SweptManagedIndexConfig.kt b/...nsearch/indexmanagement/indexstatemanagement/model/coordinator/SweptManagedIndexConfig.kt
@@ -31,7 +31,7 @@ data class SweptManagedIndexConfig(
 ) {
 
     companion object {
-        @Suppress("ComplexMethod", "UnusedPrivateMember")
+        @Suppress("ComplexMethod", "UNUSED_PARAMETER")
         @JvmStatic
         @Throws(IOException::class)
         fun parse(xcp: XContentParser, id: String = NO_ID, seqNo: Long, primaryTerm: Long): SweptManagedIndexConfig {
@@ -60,11 +60,11 @@ data class SweptManagedIndexConfig(
             }
 
             return SweptManagedIndexConfig(
-                requireNotNull(index) { "SweptManagedIndexConfig index is null" },
+                index,
                 seqNo,
                 primaryTerm,
-                requireNotNull(uuid) { "SweptManagedIndexConfig uuid is null" },
-                requireNotNull(policyID) { "SweptManagedIndexConfig policy id is null" },
+                uuid,
+                policyID,
                 policy,
                 changePolicy
             )

diff --git a/...opensearch/indexmanagement/indexstatemanagement/step/rollup/AttemptCreateRollupJobStep.kt b/...opensearch/indexmanagement/indexstatemanagement/step/rollup/AttemptCreateRollupJobStep.kt
@@ -10,6 +10,7 @@ import org.opensearch.ExceptionsHelper
 import org.opensearch.OpenSearchException
 import org.opensearch.action.support.WriteRequest
 import org.opensearch.action.support.master.AcknowledgedResponse
+import org.opensearch.client.Client
 import org.opensearch.index.engine.VersionConflictEngineException
 import org.opensearch.indexmanagement.indexstatemanagement.action.RollupAction
 import org.opensearch.indexmanagement.opensearchapi.suspendUntil
@@ -85,7 +86,7 @@ class AttemptCreateRollupJobStep(private val action: RollupAction) : Step(name)
         logger.info("Attempting to re-start the job $rollupId")
         try {
             val startRollupRequest = StartRollupRequest(rollupId)
-            val response: AcknowledgedResponse = client.suspendUntil { execute(StartRollupAction.INSTANCE, startRollupRequest, it) }
+            client.suspendUntil<Client, AcknowledgedResponse> { execute(StartRollupAction.INSTANCE, startRollupRequest, it) }
             stepStatus = StepStatus.COMPLETED
             info = mapOf("message" to getSuccessRestartMessage(rollupId, indexName))
         } catch (e: Exception) {

diff --git a/...agement/indexstatemanagement/transport/action/changepolicy/TransportChangePolicyAction.kt b/...agement/indexstatemanagement/transport/action/changepolicy/TransportChangePolicyAction.kt
@@ -288,7 +288,7 @@ class TransportChangePolicyAction @Inject constructor(
                             )
                         )
                     // if there exists a transitionTo on the ManagedIndexMetaData then we will
-                    // fail as they might not of meant to add a ChangePolicy when its on the next state
+                    // fail as they might not of meant to add a ChangePolicy when it's on the next state
                     managedIndexMetadata?.transitionTo != null ->
                         failedIndices.add(
                             FailedIndex(

diff --git a/...ensearch/indexmanagement/indexstatemanagement/transport/action/explain/ExplainResponse.kt b/...ensearch/indexmanagement/indexstatemanagement/transport/action/explain/ExplainResponse.kt
@@ -57,7 +57,7 @@ open class ExplainResponse : ActionResponse, ToXContentObject {
         indexPolicyIDs = sin.readStringList(),
         indexMetadatas = sin.readList { ManagedIndexMetaData.fromStreamInput(it) },
         totalManagedIndices = sin.readInt(),
-        enabledState = sin.readMap() as Map<String, Boolean>,
+        enabledState = sin.readMap(StreamInput::readString, StreamInput::readBoolean),
         policies = sin.readMap(StreamInput::readString, ::Policy),
         validationResults = sin.readList { ValidationResult.fromStreamInput(it) }
     )
@@ -68,7 +68,11 @@ open class ExplainResponse : ActionResponse, ToXContentObject {
         out.writeStringCollection(indexPolicyIDs)
         out.writeCollection(indexMetadatas)
         out.writeInt(totalManagedIndices)
-        out.writeMap(enabledState)
+        out.writeMap(
+            enabledState,
+            { _out, key -> _out.writeString(key) },
+            { _out, enable -> _out.writeBoolean(enable) }
+        )
         out.writeMap(
             policies,
             { _out, key -> _out.writeString(key) },

diff --git a/...main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/util/ManagedIndexUtils.kt b/...main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/util/ManagedIndexUtils.kt
@@ -517,6 +517,7 @@ fun checkMetadata(
         val t2 = when (configIndexMetadata) {
             is ManagedIndexMetaData -> configIndexMetadata.stepMetaData?.startTime
             is Map<*, *> -> {
+                @Suppress("UNCHECKED_CAST")
                 val stepMetadata = configIndexMetadata["step"] as Map<String, Any>?
                 stepMetadata?.get("start_time")
             }

diff --git a/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/util/RestHandlerUtils.kt b/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/util/RestHandlerUtils.kt
@@ -136,7 +136,8 @@ suspend fun removeClusterStateMetadatas(client: Client, logger: Logger, indices:
 }
 
 const val MASTER_TIMEOUT_DEPRECATED_MESSAGE =
-    "Parameter [master_timeout] is deprecated and will be removed in 3.0. To support inclusive language, please use [cluster_manager_timeout] instead."
+    "Parameter [master_timeout] is deprecated and will be removed in 3.0. " +
+        "To support inclusive language, please use [cluster_manager_timeout] instead."
 const val DUPLICATE_PARAMETER_ERROR_MESSAGE =
     "Please only use one of the request parameters [master_timeout, cluster_manager_timeout]."
 fun parseClusterManagerTimeout(request: RestRequest, deprecationLogger: DeprecationLogger, restActionName: String): TimeValue {

diff --git a/...n/kotlin/org/opensearch/indexmanagement/indexstatemanagement/validation/ValidateDelete.kt b/...n/kotlin/org/opensearch/indexmanagement/indexstatemanagement/validation/ValidateDelete.kt
@@ -30,7 +30,7 @@ class ValidateDelete(
         if (!deleteIndexExists(indexName) || !validIndex(indexName)) {
             return this
         }
-        val (rolloverTarget, isDataStream) = getRolloverTargetOrUpdateInfo(indexName)
+        val (rolloverTarget, _) = getRolloverTargetOrUpdateInfo(indexName)
         if (rolloverTarget != null && !notWriteIndexForDataStream(rolloverTarget, indexName)) {
             return this // can't be deleted if it's write index
         }
-Original file line number
+Diff line change
@@ Expand Up / @@ -2,6 +2,7 @@ name: Create Documentation Issue @@
     on:
       pull_request:
         types:
+          - closed
           - labeled
     env:
       PR_NUMBER: ${{ github.event.number }}
@@ Expand Down @@