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.

Sign up for GitHub

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

Jump to bottom

ILM: Get policy support wildcard name #89238

Merged
merged 9 commits into from
Sep 26, 2022
Merged

ILM: Get policy support wildcard name #89238

merged 9 commits into from
Sep 26, 2022

Conversation

weizijun
Copy link
Contributor

ILM Get policy support wildcard name is a useful feature.
We can use Get /_ilm/policy/policy* to get the wildcard result.

@elasticsearchmachine elasticsearchmachine added v8.5.0 external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label labels Aug 10, 2022
@weizijun
Copy link
Contributor Author

@dakrone can you help to reivew this PR?

@dakrone dakrone added :Data Management/ILM+SLM Index and Snapshot lifecycle management and removed needs:triage Requires assignment of a team area label labels Aug 15, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Aug 15, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@dakrone
Copy link
Member

dakrone commented Sep 7, 2022

@weizijun supporting both , and wildcards leads to a lot of very difficult edge cases around response codes. I think for now it would be better to stick with either comma-separated specific values or a wildcard. (So either GET /_ilm/policy/mypolicy1,mypolicy2 OR GET /_ilm/policy/mypolicy*.) Would you be able to make that change to this PR?

@dakrone dakrone self-assigned this Sep 7, 2022
@weizijun
Merge branch 'main' into get-ilm-support-wildcard
a6a9344 
* main: (283 commits)
  Fix integration test on Windows (elastic#89894)
  Avoiding the use of dynamic map keys in the cluster_formation results of the stable master health indicator (elastic#89842)
  Mute org.elasticsearch.tracing.apm.ApmIT.testCapturesTracesForHttpTraffic (elastic#89891)
  Fix typos in audit event types (elastic#89886)
  Synthetic _source: support histogram field (elastic#89833)
  [TSDB] Rename rollup public API to downsample  (elastic#89809)
  Format script values access (elastic#89780)
  [DOCS] Simplifies composite aggregation recommendation (elastic#89878)
  [DOCS] Update CCS compatibility matrix for 8.3 (elastic#88906)
  Fix memory leak when double invoking RestChannel.sendResponse (elastic#89873)
  [ML] Add processor autoscaling decider (elastic#89645)
  Update disk-usage.asciidoc (elastic#89709) (elastic#89874)
  Add allocation deciders in createComponents (elastic#89836)
  Mute flaky H3LatLonGeometryTest.testIndexPoints (elastic#89870)
  Fix typo in get-snapshot-status-api doc (elastic#89865)
  Picking master eligible node at random in the master stability health indicator (elastic#89841)
  Do not reuse the client after a disruption elastic#89815 (elastic#89866)
  [ML] Distribute trained model allocations across availability zones (elastic#89822)
  Increment clientCalledCount before onResponse (elastic#89858)
  AwaitsFix for elastic#89867
  ...
@weizijun
Copy link
Contributor Author

weizijun commented Sep 8, 2022

@weizijun supporting both , and wildcards leads to a lot of very difficult edge cases around response codes. I think for now it would be better to stick with either comma-separated specific values or a wildcard. (So either GET /_ilm/policy/mypolicy1,mypolicy2 OR GET /_ilm/policy/mypolicy*.) Would you be able to make that change to this PR?

Okay, I will change as you said. @dakrone ,do you mean that GET /_ilm/policy/mypolicy1*,mypolicy2* is failed? Or don't use wildcard to handle the two values?

I see that the old template action, TransportGetIndexTemplatesAction support multi names, and each name support wildcard. The new template action supports only one name. Which one is reasonable?

@dakrone
Copy link
Member

dakrone commented Sep 8, 2022

do you mean that GET /_ilm/policy/mypolicy1*,mypolicy2* is failed? Or don't use wildcard to handle the two values?

It should throw a helpful exception that says to use either comma-separated values or wildcards.

I see that the old template action, TransportGetIndexTemplatesAction support multi names, and each name support wildcard. The new template action supports only one name. Which one is reasonable?

The new one is more reasonable. The old one supports it just because we cannot make the change without breaking backwards compatibility.

@weizijun
Copy link
Contributor Author

hi, @dakrone , I have finished the change, can you help to review it?

@dakrone dakrone self-requested a review September 13, 2022 16:31
@dakrone
Copy link
Member

dakrone commented Sep 13, 2022

@elasticmachine ok to test

dakrone
dakrone reviewed Sep 13, 2022
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

@weizijun could you add a changelog file to this PR? I left a couple of other comments also

...plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportGetLifecycleAction.java Outdated Show resolved Hide resolved
x-pack/plugin/ilm/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/ilm/10_basic.yml Outdated Show resolved Hide resolved
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
@weizijun
add change log
6dbe185
@weizijun
Merge branch 'main' into get-ilm-support-wildcard
eaf78da 
* main: (186 commits)
  [DOCS] Add 8.5 release notes and fix links (elastic#90201)
  Mute DownsampleActionSingleNodeTests.testCannotRollupWhileOtherRollupInProgress (elastic#90198)
  Bump to version 8.6.0
  Increase the minimum size of the management pool to 2 (elastic#90193)
  Speed `getIntLE` from `BytesReference` (elastic#90147)
  Restrict nodes for testClusterPrimariesActive1 (elastic#90191)
  Fix bug with `BigIntArray` serialization (elastic#90142)
  Synthetic _source: test _source filtering (elastic#90138)
  Modernize cardinality agg tests (elastic#90114)
  Mute failing test (elastic#90186)
  Move assertion in ES85BloomFilterPostingsFormat to fix test (elastic#90150)
  Restrict nodes for testClusterPrimariesActive2 (elastic#90184)
  Batch index delete cluster state updates (elastic#90033)
  Register stable plugins in ActionModule (elastic#90067)
  Mute failing test (elastic#90180)
  [HealthAPI] Disk: Use _ for diagnosis id (elastic#90179)
  [HealtAPI] Disk: use shorter help URLs (elastic#90178)
  Fixing disk health indicator unit tests (elastic#90175)
  Enable the health node and the disk health indicator elastic#84811 (elastic#90085)
  Add missing Disk Indicator health api IDs (elastic#90174)
  ...
@weizijun
fixup
5a9f789
@dakrone
Copy link
Member

dakrone commented Sep 22, 2022

@elasticmachine update branch

@dakrone
Copy link
Member

dakrone commented Sep 22, 2022

The CI failures for this are transient and due to the recent 8.5.0 version bump, so I expect them to go away as those get sorted out.

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

dakrone
dakrone approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @weizijun!

@dakrone dakrone merged commit 50cfd2b into elastic:main Sep 26, 2022
@weizijun
Copy link
Contributor Author

LGTM, thanks @weizijun!

Thanks @dakrone !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakrone dakrone dakrone approved these changes

Assignees

@dakrone dakrone

Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@weizijun @elasticsearchmachine @dakrone @csoulios @elasticmachine