Accept request parameters in RestGetRollupAction and fix flakey tests (#353)

* Add option to take in request params for RestGetRollupAction

* Add GetRollupAction test and fix flakey test when getting all rollup jobs

* Fix flakey test when calling _stop API for a finished rollup job

Author: qreshi

src/main/kotlin/com/amazon/opendistroforelasticsearch/indexmanagement/rollup/action/get/GetRollupsRequest.kt

@@ -29,12 +29,13 @@ class GetRollupsRequest : ActionRequest {
     val size: Int
     val sortField: String
     val sortDirection: String
+
     constructor(
-        searchString: String = "",
-        from: Int = 0,
-        size: Int = 20,
-        sortField: String = "${Rollup.ROLLUP_TYPE}.${Rollup.ROLLUP_ID_FIELD}.keyword",
-        sortDirection: String = "asc"
+        searchString: String = DEFAULT_SEARCH_STRING,
+        from: Int = DEFAULT_FROM,
+        size: Int = DEFAULT_SIZE,
+        sortField: String = DEFAULT_SORT_FIELD,
+        sortDirection: String = DEFAULT_SORT_DIRECTION
     ) : super() {
         this.searchString = searchString
         this.from = from
@@ -62,4 +63,12 @@ class GetRollupsRequest : ActionRequest {
         out.writeString(sortField)
         out.writeString(sortDirection)
     }
+
+    companion object {
+        const val DEFAULT_SEARCH_STRING = ""
+        const val DEFAULT_FROM = 0
+        const val DEFAULT_SIZE = 20
+        const val DEFAULT_SORT_FIELD = "${Rollup.ROLLUP_TYPE}.${Rollup.ROLLUP_ID_FIELD}.keyword"
+        const val DEFAULT_SORT_DIRECTION = "asc"
+    }
 }

src/main/kotlin/com/amazon/opendistroforelasticsearch/indexmanagement/rollup/resthandler/RestGetRollupAction.kt

@@ -20,6 +20,11 @@ import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.G
 import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupRequest
 import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsAction
 import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_FROM
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_SEARCH_STRING
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_SIZE
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_SORT_DIRECTION
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_SORT_FIELD
 import org.elasticsearch.client.node.NodeClient
 import org.elasticsearch.rest.BaseRestHandler
 import org.elasticsearch.rest.RestHandler.Route
@@ -45,9 +50,21 @@ class RestGetRollupAction : BaseRestHandler() {
 
     override fun prepareRequest(request: RestRequest, client: NodeClient): RestChannelConsumer {
         val rollupID = request.param("rollupID")
+        val searchString = request.param("search", DEFAULT_SEARCH_STRING)
+        val from = request.paramAsInt("from", DEFAULT_FROM)
+        val size = request.paramAsInt("size", DEFAULT_SIZE)
+        val sortField = request.param("sortField", DEFAULT_SORT_FIELD)
+        val sortDirection = request.param("sortDirection", DEFAULT_SORT_DIRECTION)
         return RestChannelConsumer { channel ->
             if (rollupID == null || rollupID.isEmpty()) {
-                client.execute(GetRollupsAction.INSTANCE, GetRollupsRequest(), RestToXContentListener(channel))
+                val req = GetRollupsRequest(
+                    searchString,
+                    from,
+                    size,
+                    sortField,
+                    sortDirection
+                )
+                client.execute(GetRollupsAction.INSTANCE, req, RestToXContentListener(channel))
             } else {
                 val req = GetRollupRequest(rollupID, if (request.method() == HEAD) FetchSourceContext.DO_NOT_FETCH_SOURCE else null)
                 client.execute(GetRollupAction.INSTANCE, req, RestToXContentListener(channel))

src/test/kotlin/com/amazon/opendistroforelasticsearch/indexmanagement/rollup/resthandler/RestGetRollupActionIT.kt

@@ -18,6 +18,7 @@ package com.amazon.opendistroforelasticsearch.indexmanagement.rollup.resthandler
 import com.amazon.opendistroforelasticsearch.indexmanagement.IndexManagementPlugin.Companion.ROLLUP_JOBS_BASE_URI
 import com.amazon.opendistroforelasticsearch.indexmanagement.makeRequest
 import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.RollupRestTestCase
+import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.action.get.GetRollupsRequest.Companion.DEFAULT_SIZE
 import com.amazon.opendistroforelasticsearch.indexmanagement.rollup.randomRollup
 import org.elasticsearch.client.ResponseException
 import org.elasticsearch.rest.RestStatus
@@ -55,7 +56,8 @@ class RestGetRollupActionIT : RollupRestTestCase() {
     fun `test getting all rollups`() {
         val rollups = randomList(1, 15) { createRollup(randomRollup()) }
 
-        val res = client().makeRequest("GET", ROLLUP_JOBS_BASE_URI)
+        // Using a larger response size than the default in case leftover rollups prevent the ones created in this test from being returned
+        val res = client().makeRequest("GET", "$ROLLUP_JOBS_BASE_URI?size=100")
         val map = res.asMap()
         val totalRollups = map["total_rollups"] as Int
         val resRollups = map["rollups"] as List<Map<String, Any?>>
@@ -90,6 +92,28 @@ class RestGetRollupActionIT : RollupRestTestCase() {
         }
     }
 
+    @Throws(Exception::class)
+    fun `test changing response size when getting rollups`() {
+        // Ensure at least more rollup jobs than the default (20) exists
+        val rollupCount = 25
+        repeat(rollupCount) { createRollup(randomRollup()) }
+
+        // The default response size is 20, so even though 25 rollup jobs were made, at most 20 will be returned
+        var res = client().makeRequest("GET", ROLLUP_JOBS_BASE_URI)
+        var map = res.asMap()
+        var resRollups = map["rollups"] as List<Map<String, Any?>>
+
+        assertEquals("Get rollups response returned an unexpected number of jobs", DEFAULT_SIZE, resRollups.size)
+
+        // Get rollups with a larger response size
+        res = client().makeRequest("GET", "$ROLLUP_JOBS_BASE_URI?size=$rollupCount")
+        map = res.asMap()
+        resRollups = map["rollups"] as List<Map<String, Any?>>
+
+        // There can be leftover rollups from previous tests, so we will have at least rollupCount or more
+        assertEquals("Total rollups was not the same", rollupCount, resRollups.size)
+    }
+
     @Throws(Exception::class)
     fun `test checking if a rollup exists`() {
         val rollup = createRandomRollup()

src/test/kotlin/com/amazon/opendistroforelasticsearch/indexmanagement/rollup/resthandler/RestStopRollupActionIT.kt

@@ -93,6 +93,8 @@ class RestStopRollupActionIT : RollupRestTestCase() {
             val updatedRollup = getRollup(rollup.id)
             val metadata = getRollupMetadata(updatedRollup.metadataID!!)
             assertEquals("Rollup never finished", RollupMetadata.Status.FINISHED, metadata.status)
+            // Waiting for job to be disabled here to avoid version conflict exceptions later on
+            assertFalse("Job was not disabled", updatedRollup.enabled)
         }
 
         // Try to stop a finished rollup