Adds support for dynamically updatable search analyzers #290

setiah · 2020-09-09T20:56:01Z

Issue #, if available:

Description of changes:
This change is to add support for hot update of search analyzers in Elasticsearch. It exposes a new REST Api POST /_opendistro/_refresh_search_analyzers/{index}, which when invoked, refreshes those analyzers in memory which were created with is_updateable flag as true. This can be used to refresh synonym list used for search time analysis, without closing and reopening the index.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

commit c8c8233 Author: Himanshu Setia <[email protected]> Date: Tue Sep 8 19:21:58 2020 -0700 removing comments, linting changes commit 52526e8 Author: Himanshu Setia <[email protected]> Date: Tue Sep 8 19:09:30 2020 -0700 Making refresh_synonym_analyzer uri odfe compatible commit d4cc71e Author: Himanshu Setia <[email protected]> Date: Tue Sep 8 18:33:39 2020 -0700 Adding copyright disclaimer to new files commit 9af7fdf Author: Himanshu Setia <[email protected]> Date: Tue Sep 8 17:26:53 2020 -0700 Misc - removing comments, adding newline, etc. commit 1c85f18 Author: Himanshu Setia <[email protected]> Date: Tue Sep 8 16:39:26 2020 -0700 Adding _refresh_synonym_analyzer API to support dynamic update for search analyzers

codecov · 2020-09-09T21:47:18Z

Codecov Report

Merging #290 into master will decrease coverage by 0.49%.
The diff coverage is 53.33%.

@@             Coverage Diff              @@
##             master     #290      +/-   ##
============================================
- Coverage     75.32%   74.83%   -0.50%     
- Complexity      703      718      +15     
============================================
  Files            89       95       +6     
  Lines          3919     4009      +90     
  Branches        625      633       +8     
============================================
+ Hits           2952     3000      +48     
- Misses          641      683      +42     
  Partials        326      326

Impacted Files	Coverage Δ	Complexity Δ
...nt/refreshanalyzer/RefreshSearchAnalyzerRequest.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...refreshanalyzer/RestRefreshSearchAnalyzerAction.kt	`41.66% <41.66%> (ø)`	`3.00 <3.00> (?)`
...t/refreshanalyzer/RefreshSearchAnalyzerResponse.kt	`44.73% <44.73%> (ø)`	`6.00 <6.00> (?)`
...shanalyzer/TransportRefreshSearchAnalyzerAction.kt	`52.17% <52.17%> (ø)`	`1.00 <1.00> (?)`
...ent/refreshanalyzer/RefreshSearchAnalyzerAction.kt	`75.00% <75.00%> (ø)`	`2.00 <2.00> (?)`
...ticsearch/indexmanagement/IndexManagementPlugin.kt	`91.17% <100.00%> (+0.55%)`	`9.00 <0.00> (ø)`
...reshanalyzer/RefreshSearchAnalyzerShardResponse.kt	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1042dd6...02db791. Read the comment docs.

setiah · 2020-09-10T00:07:45Z

In the process of adding more UTs to close the codecov/patch gap

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerResponse.kt

...distroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerShardResponse.kt

...stroforelasticsearch/indexmanagement/refreshanalyzer/TransportRefreshSearchAnalyzerAction.kt

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerActionIT.kt

dbbaughe · 2020-09-10T20:31:31Z

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerResponse.kt

+    @Throws(IOException::class)
+    constructor(inp: StreamInput) : super(inp) {
+        val resultSize: Int = inp.readVInt()
+        for (i in 0..resultSize) {


Is this a common pattern you saw elsewhere for reading off streaminput?

Also, does this constructor only take the streaminput in the format outputted by writeTo? if so, you can change writeTo to do something like:

out.writeList(shardResponses)

and then here you can read the list and pass a class reference to the element of the list (assuming it's Writeable)

inp.readList(::RefreshSearchAnalyzerShardResponse)

You can find a similar example of this in the Monitor data class for Alerting

I found this pattern in BroadcastResponse.java, although the one that Mohammad suggested looks to be a better way. I have modified it in my latest commit. Thanks for the suggestion Mo!

dbbaughe · 2020-09-10T20:32:36Z

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerResponse.kt

+
+        val failureSize: Int = inp.readVInt()
+        for (i in 0..failureSize) {
+            shardFailures.add(FailedShardDetails(inp.readString(), inp.readInt(), inp.readString()))


nit: Make FailedShardDetails writeable and just pass in the streaminput/output

Good point. I wanted to do some refactoring on this but couldn't get to it earlier. It's done now, the new version does not use this class now.

dbbaughe · 2020-09-10T20:39:44Z

...stroforelasticsearch/indexmanagement/refreshanalyzer/TransportRefreshSearchAnalyzerAction.kt

+    private val analysisRegistry: AnalysisRegistry
+
+    @Throws(IOException::class)
+    override fun readShardResult(`in`: StreamInput): RefreshSearchAnalyzerShardResponse? {


Remove nullable return type as it seems to never be null

dbbaughe · 2020-09-10T20:40:10Z

...stroforelasticsearch/indexmanagement/refreshanalyzer/TransportRefreshSearchAnalyzerAction.kt

+    }
+
+    override fun newResponse(
+        request: RefreshSearchAnalyzerRequest?,


Just checking: can this be null or was this just from autocomplete from IDE

dbbaughe · 2020-09-10T20:40:14Z

...stroforelasticsearch/indexmanagement/refreshanalyzer/TransportRefreshSearchAnalyzerAction.kt

+        failedShards: Int,
+        results: List<RefreshSearchAnalyzerShardResponse>,
+        shardFailures: List<DefaultShardOperationFailedException>,
+        clusterState: ClusterState?


Just checking: can this be null or was this just from autocomplete from IDE

dbbaughe · 2020-09-10T20:40:42Z

...stroforelasticsearch/indexmanagement/refreshanalyzer/TransportRefreshSearchAnalyzerAction.kt

+    }
+
+    @Throws(IOException::class)
+    override fun readRequestFrom(`in`: StreamInput): RefreshSearchAnalyzerRequest? {


Remove nullable

...pendistroforelasticsearch/indexmanagement/refreshanalyzer/RestRefreshSearchAnalyzerAction.kt

dbbaughe · 2020-09-10T21:09:59Z

...ndistroforelasticsearch/indexmanagement/refreshanalyzer/RestRefreshSearchAnalyzerActionIT.kt

+            val expectedErrorMessage = mapOf(
+                    "error" to mapOf(
+                            "root_cause" to listOf<Map<String, Any>>(
+                                    mapOf("type" to "parse_exception", "reason" to "request body is required")
+                            ),
+                            "type" to "parse_exception",
+                            "reason" to "request body is required"
+                    ),
+                    "status" to 400
+            )


This seems to be not what you are trying to test? If testing for missing indices I'd expect to see an expected message for throw IllegalArgumentException("Missing indices")

the uri change caused the exception behavior to change. I will revert back the uri and change this back

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerActionIT.kt

Squashed commit of the following: commit d87842e4ccb592a39d6b6897a2b9cde9ddc2839f Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 23:36:24 2020 -0700 Reverting RefreshSearchAnalyzerResponse stream input output parsing test for fixing later commit 33ef3223acde38a64575255f7ec702f807d0502e Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 23:34:49 2020 -0700 Fixing missing indices test commit 8516d2142362c61630682bf7353e019bf9f6a9eb Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 21:59:55 2020 -0700 CR comments and added more UTs commit d306421ca462f2614756a73231baeeb56b388365 Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 20:15:59 2020 -0700 shardFailure refactoring commit da80de7a68fef05e524e624ea1a8da6afdbf11d4 Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 18:14:45 2020 -0700 Working changes after refactoring shardFailure commit be9bf2daaa31fb6444f799a8f3ed3d0383551471 Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 17:45:31 2020 -0700 Corrected response parsing for indices with some failed shards commit 256f06ed23ac4ab2d78724601a1dd7e9d1423331 Author: Himanshu Setia <[email protected]> Date: Thu Sep 10 17:12:00 2020 -0700 Working

dbbaughe · 2020-09-11T19:21:11Z

...on/opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerAction.kt

@@ -20,7 +20,7 @@ import org.elasticsearch.common.io.stream.Writeable

 class RefreshSearchAnalyzerAction : ActionType<RefreshSearchAnalyzerResponse>(NAME, reader) {
    companion object {
-        const val NAME = "indices:admin/refresh_search_analyzer"
+        const val NAME = "indices:admin/_refresh_search_analyzers"


Is it common to prefix _ on the action names?

You're right, it's not. Will change back to indices:admin/refresh_search_analyzers

Himanshu Setia added 6 commits September 8, 2020 19:24

Refactoring - API renaming

4703b95

Response parsing changes

ae21bd4

Fixed multinode response parsing issue

c68d4bc

Refactoring and logging

2f494ab

Fixing klint errors

387095a

setiah requested review from dbbaughe and qreshi September 9, 2020 23:58

qreshi reviewed Sep 10, 2020

View reviewed changes

dbbaughe reviewed Sep 10, 2020

View reviewed changes

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerActionIT.kt Outdated Show resolved Hide resolved

dbbaughe reviewed Sep 10, 2020

View reviewed changes

.../opendistroforelasticsearch/indexmanagement/refreshanalyzer/RefreshSearchAnalyzerActionIT.kt Outdated Show resolved Hide resolved

Himanshu Setia and others added 4 commits September 10, 2020 16:16

Adding UTs to validate stream parsing

2f790fc

Merge branch 'master' into hotreload2

6c33bb6

Integ test fixes by enforcing refresh during ingestion

33330d4

dbbaughe reviewed Sep 11, 2020

View reviewed changes

CR comments - reading shardResponses as List from inputStream

f4c8465

dbbaughe previously approved these changes Sep 11, 2020

View reviewed changes

Himanshu Setia added 2 commits September 11, 2020 13:31

API path change and few minor tweaks

71234c6

misc changes

4115f97

setiah dismissed dbbaughe’s stale review via 4115f97 September 11, 2020 21:25

Reverting action name to follow the code convention

02db791

qreshi approved these changes Sep 11, 2020

View reviewed changes

setiah requested review from dbbaughe, bowenlan-amzn and skkosuri-amzn September 11, 2020 23:18

bowenlan-amzn approved these changes Sep 11, 2020

View reviewed changes

setiah merged commit a095266 into opendistro-for-elasticsearch:master Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for dynamically updatable search analyzers #290

Adds support for dynamically updatable search analyzers #290

setiah commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

setiah commented Sep 10, 2020

dbbaughe Sep 10, 2020

qreshi Sep 11, 2020

setiah Sep 11, 2020

dbbaughe Sep 10, 2020

setiah Sep 11, 2020

dbbaughe Sep 10, 2020

dbbaughe Sep 10, 2020

dbbaughe Sep 10, 2020

dbbaughe Sep 10, 2020

setiah Sep 11, 2020

dbbaughe Sep 10, 2020

setiah Sep 10, 2020

dbbaughe Sep 11, 2020

setiah Sep 11, 2020

Adds support for dynamically updatable search analyzers #290

Adds support for dynamically updatable search analyzers #290

Conversation

setiah commented Sep 9, 2020 • edited Loading

codecov bot commented Sep 9, 2020 • edited Loading

Codecov Report

setiah commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setiah commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading