Add enrich policy list API #41553
Add enrich policy list API #41553
Conversation
This commit wires up the Rest calls and Transport calls for listing all enrich policies, as well as tests and rest spec additions.
hub-cap
added
>enhancement
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/bwc |
| public static final ListEnrichPolicyAction INSTANCE = new ListEnrichPolicyAction(); | ||
| public static final String NAME = "cluster:admin/xpack/enrich/list"; | ||
|
|
||
| protected ListEnrichPolicyAction() { |
There was a problem hiding this comment.
I think this constructor can be private?
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeMap(policyMap, StreamOutput::writeString, (o, value) -> value.writeTo(out)); | ||
|
|
| * @param state the cluster state | ||
| * @return a Map of <code>policyName, EnrichPolicy</code> of the policies | ||
| */ | ||
| public static Map<String, EnrichPolicy> getPolicies(ClusterState state) { |
There was a problem hiding this comment.
maybe also add a unit test for this? (now that it is public)
| { | ||
| builder.startArray("policies"); | ||
| { | ||
| for (Map.Entry<String, EnrichPolicy> entry : policyMap.entrySet()) { |
There was a problem hiding this comment.
We iterate here directly from a HashMap. The ordering of the entry set is not deterministic, so different api invocations can result in policies being returned in a different order.
In order to fix that we should make a copy here using a TreeMap:
TreeMap<String, EnrichPolicy> policyMap = new TreeMap<>(this.policyMap);
for (Map.Entry<String, EnrichPolicy> entry : policyMap.entrySet()) {
There was a problem hiding this comment.
I moved this into the constructor since the input is a map and the internal state of the object is a list.
| ); | ||
| } | ||
| ); | ||
|
|
||
| static { | ||
| PARSER.declareString(ConstructingObjectParser.constructorArg(), NAME); |
There was a problem hiding this comment.
I think this should be an optional parameter. The put transport action should set the name as it can be derived from the put policy request class. We only want to make setting the name in the url path required.
There was a problem hiding this comment.
Removed, now in NamedPolicy
|
LGTM (pending Martijn's comments) |
|
ok, readdressed all comments and worked out the |
|
|
||
| public RestListEnrichPolicyAction(final Settings settings, final RestController controller) { | ||
| super(settings); | ||
| controller.registerHandler(RestRequest.Method.GET, "/_enrich/policy", this); |
There was a problem hiding this comment.
if w go with this then i suggest we change all of the URIs, which i like, but I assumed we did not want to go against for now. This is how I view it. You are enacting on a single or multiple policies at any time. So, /policies gives you the list, and /policies/foo gives your single foo policy back, but the resource is the same, you are just saying "give me a singlular one of these policies". Otherwise I think we should keep it the same as policy across the board. I prefer the former, but its possible we dont want to go that route until more actual API and versioning conversations happen.
There was a problem hiding this comment.
ok :) Lets leave this for now and decide whether we want this in a later stage of development.
Btw I just check get mappings api (returns a hash and not a list) and there both singular and plural is supported. But it looks like most apis don't mix plural with singular nouns when it comes returning a single item or multiple (get pipeline, get repository, get tasks etc). Anyhow, language wise (i'm not a native speaker) plural made sense to me.
There was a problem hiding this comment.
IMHO plural makes sesne for all cases. It reads easier to me, a list of policies GET /_enrich/policies, a single policy from the list of policies GET /_enrich/policies/foo
This commit wires up the Rest calls and Transport calls for listing all enrich policies, as well as tests and rest spec additions.
This commit wires up the Rest calls and Transport calls for listing all
enrich policies, as well as tests and rest spec additions.