Implemented filtering on the ISM eplain API #998
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Joshua152
requested review from
bowenlan-amzn,
getsaurabh02,
lezzago,
praveensameneni,
xluo-aws,
gaobinlong,
Hailong-am,
amsiglan,
sbcd90,
eirsep,
AWSHurneyt and
jowg-amazon
as code owners
Joshua152
force-pushed
the
osci
branch
2 times, most recently
from
3ef7642 to
828f8d2
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
============================================
- Coverage 74.80% 74.41% -0.39%
+ Complexity 2787 2785 -2
============================================
Files 366 367 +1
Lines 16422 16502 +80
Branches 2342 2357 +15
============================================
- Hits 12284 12280 -4
- Misses 2836 2919 +83
- Partials 1302 1303 +1 ☔ View full report in Codecov by Sentry. |
|
@Joshua152 thanks, this is good progress. |
Signed-off-by: Joshua Au <[email protected]>
Signed-off-by: Joshua Au <[email protected]>
Signed-off-by: Joshua Au <[email protected]>
Signed-off-by: Joshua Au <[email protected]>
|
@Joshua152 maybe pull the change from #1022 back to this PR, and let's wrap on this. 😃 |
Signed-off-by: Joshua Au <[email protected]>
Signed-off-by: Joshua Au <[email protected]>
| @@ -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 | |||
| @@ -21,6 +22,7 @@ class ExplainRequest : ActionRequest { | |||
| val local: Boolean | |||
| val clusterManagerTimeout: TimeValue | |||
| val searchParams: SearchParams | |||
| val explainFilter: ExplainFilter | |||
There was a problem hiding this comment.
Prefer explainFilter to be nullable.
|
|
||
| @JvmStatic | ||
| @Throws(IOException::class) | ||
| fun parse(xcp: XContentParser): ExplainFilter { |
There was a problem hiding this comment.
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
src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ExplainFilter.kt
Outdated
Show resolved
Hide resolved
| return boolQuery | ||
| } | ||
|
|
||
| fun validMetaData(metaData: ManagedIndexMetaData): Boolean { |
There was a problem hiding this comment.
Better name as filterByMetadata? or even byMetadata since the class name already contains filter in it
| } | ||
|
|
||
| fun validMetaData(metaData: ManagedIndexMetaData): Boolean { | ||
| var ok = true |
There was a problem hiding this comment.
ok is a little misleading here 😃 sometimes it's actually not ok
| ok = false | ||
| } | ||
|
|
||
| if (failed != null && (actionMetaData == null || actionMetaData.failed != failed)) { |
There was a problem hiding this comment.
Also check the retry.failed field in metadata here, if it's true, meaning this job is failed
Comment on lines
302
to
303
| ok = request.explainFilter.validMetaData(metaData) | ||
| if (!ok) { |
There was a problem hiding this comment.
nit: if (request.explainFilter.validMetaData(metaData))
| response.responses.associate { it.id to getMetadata(it.response)?.toMap() } | ||
|
|
||
| metadataMap = metadataMap.filter { (_, value) -> |
There was a problem hiding this comment.
Need to add IT in RestExplainIT for this, feel free to do this in another PR
Signed-off-by: Joshua Au <[email protected]>
Signed-off-by: Joshua Au <[email protected]>
jowg-amazon
reviewed
Dec 12, 2023
| if (value != null) { | ||
| val metaData = ManagedIndexMetaData.fromMap(value) | ||
|
|
||
| if (!request.explainFilter.byMetaData(metaData)) { |
There was a problem hiding this comment.
Why are we removing metaData.index from indexNames and indexNamesToUUIDs?
There was a problem hiding this comment.
When creating the actual response, all the index names are looped over. So, in order to remove the filtered indices, they must be removed from indexNames.
jowg-amazon
reviewed
Dec 12, 2023
| @@ -52,6 +56,14 @@ class RestExplainAction : BaseRestHandler() { | |||
| ReplacedRoute( | |||
| GET, "$EXPLAIN_BASE_URI/{index}", | |||
| GET, "$LEGACY_EXPLAIN_BASE_URI/{index}" | |||
| ), | |||
| ReplacedRoute( | |||
There was a problem hiding this comment.
Are we able to use explain API with filter enabled across all indices and on specific indices?
There was a problem hiding this comment.
Yes, it works for both
bowenlan-amzn
merged commit 21f5f8d
into
opensearch-project:main
22 of 25 checks passed
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/index-management/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/index-management/backport-2.x
# Create a new branch
git switch --create backport/backport-998-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 21f5f8d40377d91eb45d3af3747fffde5fe06525
# Push it to GitHub
git push --set-upstream origin backport/backport-998-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/index-management/backport-2.xThen, create a pull request where the |
|
@Joshua152 this has been merged in, but the backport failed, can you manually apply this change to 2.x branch and create a PR please? |
Joshua152
added a commit
to Joshua152/index-management
that referenced
this pull request
Dec 18, 2023
Signed-off-by: Joshua Au <[email protected]>
Joshua152
added a commit
to Joshua152/index-management
that referenced
this pull request
Dec 26, 2023
* Implemented filtering on the ISM eplain API Signed-off-by: Joshua Au <[email protected]> * Fixed tests for ExplainRequest Signed-off-by: Joshua Au <[email protected]> * Added filtering on query and metadata map Signed-off-by: Joshua Au <[email protected]> * Filtered on indexNames in metadata Signed-off-by: Joshua Au <[email protected]> * Fixed github workflow check errors Signed-off-by: Joshua Au <[email protected]> * Removed debugging comments Signed-off-by: Joshua Au <[email protected]> * Updated code styling to make more clear Signed-off-by: Joshua Au <[email protected]> * Refactored code to match suggestions Signed-off-by: Joshua Au <[email protected]> --------- Signed-off-by: Joshua Au <[email protected]> Co-authored-by: bowenlan-amzn <[email protected]>
jowg-amazon
pushed a commit
that referenced
this pull request
Jan 4, 2024
* Backport issue #998 to 2.x Signed-off-by: Joshua Au <[email protected]> * Issue651 tests (#1046) * Implemented filtering on the ISM eplain API Signed-off-by: Joshua Au <[email protected]> * Fixed tests for ExplainRequest Signed-off-by: Joshua Au <[email protected]> * Added filtering on query and metadata map Signed-off-by: Joshua Au <[email protected]> * Filtered on indexNames in metadata Signed-off-by: Joshua Au <[email protected]> * Fixed github workflow check errors Signed-off-by: Joshua Au <[email protected]> * Removed debugging comments Signed-off-by: Joshua Au <[email protected]> * Updated code styling to make more clear Signed-off-by: Joshua Au <[email protected]> * Refactored code to match suggestions Signed-off-by: Joshua Au <[email protected]> * Added test case for the ExplainFilter.byMetaData and parse methods Signed-off-by: Joshua Au <[email protected]> * Started implementation of explain filter IT Signed-off-by: Joshua Au <[email protected]> * Implemented test explain filter method Signed-off-by: Joshua Au <[email protected]> * Implemented explain filter test on failure Signed-off-by: Joshua Au <[email protected]> * Cleaned up log statements Signed-off-by: Joshua Au <[email protected]> * Added explain filter test for success Signed-off-by: Joshua Au <[email protected]> * Fixed lint errors Signed-off-by: Joshua Au <[email protected]> * Removed policy from index to fix flaky tests Signed-off-by: Joshua Au <[email protected]> --------- Signed-off-by: Joshua Au <[email protected]> Signed-off-by: bowenlan-amzn <[email protected]> Co-authored-by: bowenlan-amzn <[email protected]> * Fixed content type import Signed-off-by: Joshua Au <[email protected]> * Implemented filtering on the ISM eplain API (#998) * Implemented filtering on the ISM eplain API Signed-off-by: Joshua Au <[email protected]> * Fixed tests for ExplainRequest Signed-off-by: Joshua Au <[email protected]> * Added filtering on query and metadata map Signed-off-by: Joshua Au <[email protected]> * Filtered on indexNames in metadata Signed-off-by: Joshua Au <[email protected]> * Fixed github workflow check errors Signed-off-by: Joshua Au <[email protected]> * Removed debugging comments Signed-off-by: Joshua Au <[email protected]> * Updated code styling to make more clear Signed-off-by: Joshua Au <[email protected]> * Refactored code to match suggestions Signed-off-by: Joshua Au <[email protected]> --------- Signed-off-by: Joshua Au <[email protected]> Co-authored-by: bowenlan-amzn <[email protected]> * Issue651 tests (#1046) * Implemented filtering on the ISM eplain API Signed-off-by: Joshua Au <[email protected]> * Fixed tests for ExplainRequest Signed-off-by: Joshua Au <[email protected]> * Added filtering on query and metadata map Signed-off-by: Joshua Au <[email protected]> * Filtered on indexNames in metadata Signed-off-by: Joshua Au <[email protected]> * Fixed github workflow check errors Signed-off-by: Joshua Au <[email protected]> * Removed debugging comments Signed-off-by: Joshua Au <[email protected]> * Updated code styling to make more clear Signed-off-by: Joshua Au <[email protected]> * Refactored code to match suggestions Signed-off-by: Joshua Au <[email protected]> * Added test case for the ExplainFilter.byMetaData and parse methods Signed-off-by: Joshua Au <[email protected]> * Started implementation of explain filter IT Signed-off-by: Joshua Au <[email protected]> * Implemented test explain filter method Signed-off-by: Joshua Au <[email protected]> * Implemented explain filter test on failure Signed-off-by: Joshua Au <[email protected]> * Cleaned up log statements Signed-off-by: Joshua Au <[email protected]> * Added explain filter test for success Signed-off-by: Joshua Au <[email protected]> * Fixed lint errors Signed-off-by: Joshua Au <[email protected]> * Removed policy from index to fix flaky tests Signed-off-by: Joshua Au <[email protected]> --------- Signed-off-by: Joshua Au <[email protected]> Signed-off-by: bowenlan-amzn <[email protected]> Co-authored-by: bowenlan-amzn <[email protected]> * Backport explain filter tests * Sleep thread to allow jobs to finish Signed-off-by: Joshua Au <[email protected]> --------- Signed-off-by: Joshua Au <[email protected]> Signed-off-by: bowenlan-amzn <[email protected]> Co-authored-by: bowenlan-amzn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
#651
Description of changes:
Added POST method to ISM explain API to enable filtering on the policies. This does the same thing as GET, except adds in filtering. The request body should look as follows:
"filter": {
"policy_id": "my_id",
"state": "my_state_name",
"action_type": "my_action_type"
}
The filter only returns policies for which they match every filter given. If a filter is left out (ex. "policy_id"), any of that filter will be allowed.
CheckList:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.