Skip to content
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.

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

Implemented filtering on the ISM eplain API #998

Merged
merged 9 commits into from
Dec 14, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import org.opensearch.indexmanagement.spi.indexstatemanagement.model.ManagedInde
import java.io.ByteArrayInputStream
import java.nio.charset.StandardCharsets

data class StateMetaData(
data class
Copy link
Member

Choose a reason for hiding this comment

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

Can change this back

StateMetaData(
val name: String,
val startTime: Long
) : Writeable, ToXContentFragment {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.indexmanagement.indexstatemanagement.model

import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.common.io.stream.Writeable
import org.opensearch.core.xcontent.XContentParser
import org.opensearch.core.xcontent.XContentParser.Token
import org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken
import org.opensearch.index.query.BoolQueryBuilder
import org.opensearch.index.query.Operator
import org.opensearch.index.query.QueryBuilders
import org.opensearch.indexmanagement.indexstatemanagement.util.MANAGED_INDEX_POLICY_ID_FIELD
import org.opensearch.indexmanagement.spi.indexstatemanagement.model.ManagedIndexMetaData
import java.io.IOException

data class ExplainFilter(
val policyID: String?,
val state: String?,
val actionType: String?,
val failed: Boolean?
) : Writeable {

constructor() : this(
policyID = null,
state = null,
actionType = null,
failed = null
)

@Throws(IOException::class)
constructor(sin: StreamInput) : this(
policyID = sin.readOptionalString(),
state = sin.readOptionalString(),
actionType = sin.readOptionalString(),
failed = sin.readOptionalBoolean()
)

@Throws(IOException::class)
override fun writeTo(out: StreamOutput) {
out.writeOptionalString(policyID)
out.writeOptionalString(state)
out.writeOptionalString(actionType)
out.writeOptionalBoolean(failed)
}

fun convertToBoolQueryBuilder(): BoolQueryBuilder {
val boolQuery = QueryBuilders.boolQuery()
bowenlan-amzn marked this conversation as resolved.
Show resolved Hide resolved

if (policyID != null) {
boolQuery.must(

Check warning on line 55 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L55

Added line #L55 was not covered by tests
QueryBuilders
.queryStringQuery(policyID)
.field(MANAGED_INDEX_POLICY_ID_FIELD)
.defaultOperator(Operator.AND)

Check warning on line 59 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L57-L59

Added lines #L57 - L59 were not covered by tests
bowenlan-amzn marked this conversation as resolved.
Show resolved Hide resolved
)
}

return boolQuery
}

fun validMetaData(metaData: ManagedIndexMetaData): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Better name as filterByMetadata? or even byMetadata since the class name already contains filter in it

var ok = true
Copy link
Member

Choose a reason for hiding this comment

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

ok is a little misleading here 😃 sometimes it's actually not ok


val stateMetaData = metaData.stateMetaData
if (state != null && (stateMetaData == null || stateMetaData.name != state)) {
ok = false

Check warning on line 71 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L71

Added line #L71 was not covered by tests
}

val actionMetaData = metaData.actionMetaData
if (actionType != null && (actionMetaData == null || actionMetaData.name != actionType)) {
ok = false

Check warning on line 76 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L76

Added line #L76 was not covered by tests
}

if (failed != null && (actionMetaData == null || actionMetaData.failed != failed)) {
Copy link
Member

Choose a reason for hiding this comment

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

Also check the retry.failed field in metadata here, if it's true, meaning this job is failed

ok = false

Check warning on line 80 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L80

Added line #L80 was not covered by tests
}

return ok
}

companion object {
const val FILTER_FIELD = "filter"
const val POLICY_ID_FIELD = "policy_id"
const val STATE_FIELD = "state"
const val ACTION_FIELD = "action_type"
const val FAILED_FIELD = "failed"

@JvmStatic
@Throws(IOException::class)
fun parse(xcp: XContentParser): ExplainFilter {
Copy link
Member

Choose a reason for hiding this comment

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

Please add unit tests for the codecov warning. Feel free to add in another PR
Let me know if you need any guidance on writing UTs

var policyID: String? = null
var state: String? = null
var actionType: String? = null
var failed: Boolean? = null

Check warning on line 99 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L96-L99

Added lines #L96 - L99 were not covered by tests

ensureExpectedToken(Token.START_OBJECT, xcp.currentToken(), xcp)

Check warning on line 101 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L101

Added line #L101 was not covered by tests
while (xcp.nextToken() != Token.END_OBJECT) {
val fieldName = xcp.currentName()
xcp.nextToken()

Check warning on line 104 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L103-L104

Added lines #L103 - L104 were not covered by tests

when (fieldName) {

Check warning on line 106 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L106

Added line #L106 was not covered by tests
FILTER_FIELD -> {
ensureExpectedToken(Token.START_OBJECT, xcp.currentToken(), xcp)

Check warning on line 108 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L108

Added line #L108 was not covered by tests
while (xcp.nextToken() != Token.END_OBJECT) {
val filter = xcp.currentName()
xcp.nextToken()

Check warning on line 111 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L110-L111

Added lines #L110 - L111 were not covered by tests

when (filter) {
POLICY_ID_FIELD -> policyID = xcp.text()
STATE_FIELD -> state = xcp.text()
ACTION_FIELD -> actionType = xcp.text()
FAILED_FIELD -> failed = xcp.booleanValue()

Check warning on line 117 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L114-L117

Added lines #L114 - L117 were not covered by tests
}
}
}
}
}

return ExplainFilter(policyID, state, actionType, failed)

Check warning on line 124 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt#L124

Added line #L124 was not covered by tests
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
import org.opensearch.client.node.NodeClient
import org.opensearch.core.common.Strings
import org.opensearch.common.logging.DeprecationLogger
import org.opensearch.core.xcontent.XContentParser.Token
import org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken
import org.opensearch.indexmanagement.IndexManagementPlugin.Companion.ISM_BASE_URI
import org.opensearch.indexmanagement.IndexManagementPlugin.Companion.LEGACY_ISM_BASE_URI
import org.opensearch.indexmanagement.indexstatemanagement.model.ExplainFilter
import org.opensearch.indexmanagement.indexstatemanagement.transport.action.explain.ExplainAction
import org.opensearch.indexmanagement.indexstatemanagement.transport.action.explain.ExplainRequest
import org.opensearch.indexmanagement.indexstatemanagement.util.DEFAULT_EXPLAIN_VALIDATE_ACTION
Expand All @@ -28,6 +31,7 @@
import org.opensearch.rest.RestHandler.Route
import org.opensearch.rest.RestRequest
import org.opensearch.rest.RestRequest.Method.GET
import org.opensearch.rest.RestRequest.Method.POST
import org.opensearch.rest.action.RestToXContentListener

private val log = LogManager.getLogger(RestExplainAction::class.java)
Expand All @@ -52,6 +56,14 @@
ReplacedRoute(
GET, "$EXPLAIN_BASE_URI/{index}",
GET, "$LEGACY_EXPLAIN_BASE_URI/{index}"
),
ReplacedRoute(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to use explain API with filter enabled across all indices and on specific indices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works for both

POST, EXPLAIN_BASE_URI,
POST, LEGACY_EXPLAIN_BASE_URI
),
ReplacedRoute(
POST, "$EXPLAIN_BASE_URI/{index}",
POST, "$LEGACY_EXPLAIN_BASE_URI/{index}"
)
)
}
Expand All @@ -69,6 +81,14 @@

val indexType = request.param(TYPE_PARAM_KEY, DEFAULT_INDEX_TYPE)

val explainFilter = if (request.method() == RestRequest.Method.POST) {
val xcp = request.contentParser()
ensureExpectedToken(Token.START_OBJECT, xcp.nextToken(), xcp)
ExplainFilter.parse(xcp)

Check warning on line 87 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/resthandler/RestExplainAction.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/resthandler/RestExplainAction.kt#L85-L87

Added lines #L85 - L87 were not covered by tests
} else {
ExplainFilter()
}

val clusterManagerTimeout = parseClusterManagerTimeout(
request, DeprecationLogger.getLogger(RestExplainAction::class.java), name
)
Expand All @@ -78,6 +98,7 @@
request.paramAsBoolean("local", false),
clusterManagerTimeout,
searchParams,
explainFilter,
request.paramAsBoolean(SHOW_POLICY_QUERY_PARAM, DEFAULT_EXPLAIN_SHOW_POLICY),
request.paramAsBoolean(SHOW_VALIDATE_ACTION, DEFAULT_EXPLAIN_VALIDATE_ACTION),
indexType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.common.unit.TimeValue
import org.opensearch.indexmanagement.common.model.rest.SearchParams
import org.opensearch.indexmanagement.indexstatemanagement.model.ExplainFilter
import org.opensearch.indexmanagement.indexstatemanagement.util.DEFAULT_INDEX_TYPE
import java.io.IOException

Expand All @@ -21,6 +22,7 @@ class ExplainRequest : ActionRequest {
val local: Boolean
val clusterManagerTimeout: TimeValue
val searchParams: SearchParams
val explainFilter: ExplainFilter
Copy link
Member

Choose a reason for hiding this comment

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

Prefer explainFilter to be nullable.

val showPolicy: Boolean
val validateAction: Boolean
val indexType: String
Expand All @@ -31,6 +33,7 @@ class ExplainRequest : ActionRequest {
local: Boolean,
clusterManagerTimeout: TimeValue,
searchParams: SearchParams,
explainFilter: ExplainFilter,
showPolicy: Boolean,
validateAction: Boolean,
indexType: String
Expand All @@ -39,6 +42,7 @@ class ExplainRequest : ActionRequest {
this.local = local
this.clusterManagerTimeout = clusterManagerTimeout
this.searchParams = searchParams
this.explainFilter = explainFilter
this.showPolicy = showPolicy
this.validateAction = validateAction
this.indexType = indexType
Expand All @@ -50,6 +54,7 @@ class ExplainRequest : ActionRequest {
local = sin.readBoolean(),
clusterManagerTimeout = sin.readTimeValue(),
searchParams = SearchParams(sin),
explainFilter = ExplainFilter(sin),
showPolicy = sin.readBoolean(),
validateAction = sin.readBoolean(),
indexType = sin.readString()
Expand All @@ -72,6 +77,7 @@ class ExplainRequest : ActionRequest {
out.writeBoolean(local)
out.writeTimeValue(clusterManagerTimeout)
searchParams.writeTo(out)
explainFilter.writeTo(out)
out.writeBoolean(showPolicy)
out.writeBoolean(validateAction)
out.writeString(indexType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,11 @@
private fun getSearchMetadataRequest(params: SearchParams, indexUUIDs: List<String>, searchSize: Int): SearchRequest {
val sortBuilder = params.getSortBuilder()

val queryBuilder = QueryBuilders.boolQuery()
val queryBuilder = request.explainFilter.convertToBoolQueryBuilder()
.must(
QueryBuilders
.queryStringQuery(params.queryString)
.defaultField(MANAGED_INDEX_NAME_KEYWORD_FIELD)
.field(MANAGED_INDEX_NAME_KEYWORD_FIELD)
.defaultOperator(Operator.AND)
).filter(QueryBuilders.termsQuery(MANAGED_INDEX_INDEX_UUID_FIELD, indexUUIDs))

Expand Down Expand Up @@ -291,8 +291,25 @@
mgetMetadataReq,
object : ActionListener<MultiGetResponse> {
override fun onResponse(response: MultiGetResponse) {
val metadataMap: Map<ManagedIndexMetadataDocUUID, ManagedIndexMetadataMap?> =
var metadataMap: Map<ManagedIndexMetadataDocUUID, ManagedIndexMetadataMap?> =
response.responses.associate { it.id to getMetadata(it.response)?.toMap() }

metadataMap = metadataMap.filter { (_, value) ->
Copy link
Member

Choose a reason for hiding this comment

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

Need to add IT in RestExplainIT for this, feel free to do this in another PR

var ok = false

if (value != null) {
val metaData = ManagedIndexMetaData.fromMap(value)
ok = request.explainFilter.validMetaData(metaData)
if (!ok) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: if (request.explainFilter.validMetaData(metaData))

indexNames.remove(metaData.index)
indexNamesToUUIDs.remove(metaData.index)
totalManagedIndices--

Check warning on line 306 in src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/transport/action/explain/TransportExplainAction.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/transport/action/explain/TransportExplainAction.kt#L304-L306

Added lines #L304 - L306 were not covered by tests
}
}

ok
}

buildResponse(indexNamesToUUIDs, metadataMap, clusterStateIndexMetadatas, threadContext)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const val MANAGED_INDEX_FIELD = "managed_index"
const val MANAGED_INDEX_NAME_KEYWORD_FIELD = "$MANAGED_INDEX_FIELD.name.keyword"
const val MANAGED_INDEX_INDEX_FIELD = "$MANAGED_INDEX_FIELD.index"
const val MANAGED_INDEX_INDEX_UUID_FIELD = "$MANAGED_INDEX_FIELD.index_uuid"
const val MANAGED_INDEX_POLICY_ID_FIELD = "$MANAGED_INDEX_FIELD.policy_id"

const val DEFAULT_JOB_SORT_FIELD = MANAGED_INDEX_INDEX_FIELD
const val DEFAULT_POLICY_SORT_FIELD = "policy.policy_id.keyword"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import org.opensearch.common.io.stream.BytesStreamOutput
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.common.unit.TimeValue
import org.opensearch.indexmanagement.common.model.rest.SearchParams
import org.opensearch.indexmanagement.indexstatemanagement.model.ExplainFilter
import org.opensearch.indexmanagement.indexstatemanagement.util.DEFAULT_INDEX_TYPE
import org.opensearch.test.OpenSearchTestCase

Expand All @@ -19,9 +20,10 @@ class ExplainRequestTests : OpenSearchTestCase() {
val local = true
val clusterManagerTimeout = TimeValue.timeValueSeconds(30)
val params = SearchParams(0, 20, "sort-field", "asc", "*")
val filter = ExplainFilter()
val showPolicy = false
val showValidationResult = false
val req = ExplainRequest(indices, local, clusterManagerTimeout, params, showPolicy, showValidationResult, DEFAULT_INDEX_TYPE)
val req = ExplainRequest(indices, local, clusterManagerTimeout, params, filter, showPolicy, showValidationResult, DEFAULT_INDEX_TYPE)

val out = BytesStreamOutput()
req.writeTo(out)
Expand All @@ -36,9 +38,10 @@ class ExplainRequestTests : OpenSearchTestCase() {
val local = true
val clusterManagerTimeout = TimeValue.timeValueSeconds(30)
val params = SearchParams(0, 20, "sort-field", "asc", "*")
val filter = ExplainFilter()
val showPolicy = false
val showValidationResult = false
val req = ExplainRequest(indices, local, clusterManagerTimeout, params, showPolicy, showValidationResult, "non-existent-index-type")
val req = ExplainRequest(indices, local, clusterManagerTimeout, params, filter, showPolicy, showValidationResult, "non-existent-index-type")

val actualException: String? = req.validate()?.validationErrors()?.firstOrNull()
val expectedException: String = ExplainRequest.MULTIPLE_INDICES_CUSTOM_INDEX_TYPE_ERROR
Expand Down
Loading