Return reloaded analyzers in _reload_search_ananlyzer response #43813
Conversation
Currently the repsonse of the "_reload_search_analyzer" endpoint contains the index names and nodeIds of indices were analyzers reloading was triggered. This change add the names of the search-time analyzers that were reloaded. Closes elastic#43804
cbuescher
added
>enhancement
:Search Relevance/Analysis
|
Pinging @elastic/es-search |
| "failed" : 0 | ||
| }, | ||
| "reloaded_nodes" : [ |
There was a problem hiding this comment.
A few thoughts around the structure of this response:
- I found it a little confusing that this section was named
reloaded_nodes, since it's centered around indices instead of nodes (and we call these sectionsIndexDetailsin the Java response). Maybereloaded_indiceswould make more sense, or even justindices? - When there is one response section per index, we often return a map instead of a list, with the index name as the map key. For example, the 'get index' and 'get mapping' APIs follow this response format.
There was a problem hiding this comment.
Yes, the format of the response here is a bit different. The reason for this ist that although reloading is centered around indices, it needs to be executed on each node an index shard might live on (and not only once on each shard in a replica set). So this is potentially more than one node per index. While I think the list of nodeIds that were part of reloading for each index is not that important to the average user, it can provide valuable debug information e.g. for UI clients later on.
Regarding your second point: I know we use index names as keys in several places but if not necessary I consciously try to avoid it. I makes parsing of responses more difficult because the parser need to deal with an unbounded set of keys as field names. While we mange to get around it in the places you mentioned, I try to avoid it if possible in order to make the rest response easier to parse.
There was a problem hiding this comment.
although reloading is centered around indices, it needs to be executed on each node an index shard might live on
That makes sense to me, I was just wondering about the name of the top-level key for the section (currently reloaded_nodes). The name seemed fitting when this section only contained node IDs, but now that it contains analyzer information as well, maybe there could be a better name? In Java this is called reloadedIndicesDetails, so I was bit surprised to see it called reloaded_nodes in the REST response as opposed to something like reloaded_indices.
I know we use index names as keys in several places but if not necessary I consciously try to avoid it.
I agree it makes parsing more difficult! To me it would be best to maintain consistency across APIs (even if it makes parsing a little harder) as opposed to having a mix of styles, since this is least surprising to users. However this may be a bigger conversation, and the API is marked as experimental and can be changed, so it doesn't seem like it should prevent merging.
There was a problem hiding this comment.
I see what you mean now with the top-level key, sorry that I missed that part. Will change to something more neutral and probably also allign the java API to that new key a bit.
|
@jtibshirani thanks for the review, I tried answering your questions around the response format and to explain the decisions involved on my side. Let me know if you have other suggestions for changes. |
| "failed" : 0 | ||
| }, | ||
| "reloaded_nodes" : [ |
There was a problem hiding this comment.
although reloading is centered around indices, it needs to be executed on each node an index shard might live on
That makes sense to me, I was just wondering about the name of the top-level key for the section (currently reloaded_nodes). The name seemed fitting when this section only contained node IDs, but now that it contains analyzer information as well, maybe there could be a better name? In Java this is called reloadedIndicesDetails, so I was bit surprised to see it called reloaded_nodes in the REST response as opposed to something like reloaded_indices.
I know we use index names as keys in several places but if not necessary I consciously try to avoid it.
I agree it makes parsing more difficult! To me it would be best to maintain consistency across APIs (even if it makes parsing a little harder) as opposed to having a mix of styles, since this is least surprising to users. However this may be a bigger conversation, and the API is marked as experimental and can be changed, so it doesn't seem like it should prevent merging.
|
@jtibshirani sorry I missread your earlier comment about the top-level response key, it probably was a bit later. I changed ""reloaded_nodes" to the more generic "reload_details" because that makes more sense now that there is more information grouped under it. I also changed the Javas API methods to something closer to the rest section ( |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
Currently the repsonse of the "_reload_search_analyzer" endpoint contains the index names and nodeIds of indices were analyzers reloading was triggered. This change add the names of the search-time analyzers that were reloaded. Closes #43804
Currently the repsonse of the "_reload_search_analyzer" endpoint contains the
index names and nodeIds of indices were analyzers reloading was triggered. This
change add the names of the search-time analyzers that were reloaded.
Closes #43804