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

Added minimum for search.cancel_after_time_interval setting for rollups #1026

Merged
merged 6 commits into from
Nov 19, 2023
Merged

Added minimum for search.cancel_after_time_interval setting for rollups #1026

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d98fd53
Added minimum for search.cancel_after_time_interval setting for rollups
Joshua152 Oct 31, 2023
166c303
Added constant for cancel_after_time_interval for rollup search
Joshua152 Oct 31, 2023
687c7e7
Handled case of default value for cancel interval
Joshua152 Nov 8, 2023
a259d1c
Added comment explanation for default rollup cancel after time interval
Joshua152 Nov 8, 2023
a37c248
Fixed github workflow checks
Joshua152 Nov 8, 2023
24a578d
Merge branch 'main' into issue710
bowenlan-amzn Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupSearchService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,17 @@
import org.opensearch.action.bulk.BackoffPolicy
import org.opensearch.action.search.SearchPhaseExecutionException
import org.opensearch.action.search.SearchResponse
import org.opensearch.action.search.TransportSearchAction.SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING
import org.opensearch.client.Client
import org.opensearch.cluster.service.ClusterService
import org.opensearch.core.common.breaker.CircuitBreakingException
import org.opensearch.common.settings.Settings
import org.opensearch.common.unit.TimeValue
import org.opensearch.indexmanagement.opensearchapi.retry
import org.opensearch.indexmanagement.opensearchapi.suspendUntil
import org.opensearch.indexmanagement.rollup.model.Rollup
import org.opensearch.indexmanagement.rollup.model.RollupMetadata
import org.opensearch.indexmanagement.rollup.settings.RollupSettings.Companion.MINIMUM_CANCEL_AFTER_TIME_INTERVAL_MINUTES
import org.opensearch.indexmanagement.rollup.settings.RollupSettings.Companion.ROLLUP_SEARCH_BACKOFF_COUNT
import org.opensearch.indexmanagement.rollup.settings.RollupSettings.Companion.ROLLUP_SEARCH_BACKOFF_MILLIS
import org.opensearch.indexmanagement.rollup.util.getRollupSearchRequest
Expand All @@ -44,10 +47,16 @@
@Volatile private var retrySearchPolicy =
BackoffPolicy.constantBackoff(ROLLUP_SEARCH_BACKOFF_MILLIS.get(settings), ROLLUP_SEARCH_BACKOFF_COUNT.get(settings))

@Volatile private var cancelAfterTimeInterval = SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING.get(settings)

init {
clusterService.clusterSettings.addSettingsUpdateConsumer(ROLLUP_SEARCH_BACKOFF_MILLIS, ROLLUP_SEARCH_BACKOFF_COUNT) { millis, count ->
retrySearchPolicy = BackoffPolicy.constantBackoff(millis, count)
}

clusterService.clusterSettings.addSettingsUpdateConsumer(SEARCH_CANCEL_AFTER_TIME_INTERVAL_SETTING) {
cancelAfterTimeInterval = it

Check warning on line 58 in src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupSearchService.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupSearchService.kt#L58 

Added line #L58 was not covered by tests
}
}

// TODO: Failed shouldn't process? How to recover from failed -> how does a user retry a failed rollup
Expand Down Expand Up @@ -103,7 +112,12 @@
"Composite search failed for rollup, retrying [#${retryCount - 1}] -" +
" reducing page size of composite aggregation from ${job.pageSize} to $pageSize"
)
search(job.copy(pageSize = pageSize).getRollupSearchRequest(metadata), listener)

val searchRequest = job.copy(pageSize = pageSize).getRollupSearchRequest(metadata)
val cancelTimeoutTimeValue = TimeValue.timeValueMinutes(getCancelAfterTimeInterval(cancelAfterTimeInterval.minutes))
searchRequest.cancelAfterTimeInterval = cancelTimeoutTimeValue

search(searchRequest, listener)
}
}
)
Expand Down Expand Up @@ -132,6 +146,16 @@
RollupSearchResult.Failure(cause = e)
}
}

private fun getCancelAfterTimeInterval(givenInterval: Long): Long {
// The default value for the cancelAfterTimeInterval is -1 and so, in this case
// we should ignore processing on the value
if (givenInterval == -1L) {
return givenInterval

Check warning on line 154 in src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupSearchService.kt

View check run for this annotation

Codecov / codecov/patch

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupSearchService.kt#L154 

Added line #L154 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

can add a unit test for code coverage CI

}

return max(cancelAfterTimeInterval.minutes(), MINIMUM_CANCEL_AFTER_TIME_INTERVAL_MINUTES)
}
}

sealed class RollupSearchResult {
Expand Down
1 change: 1 addition & 0 deletions src/main/kotlin/org/opensearch/indexmanagement/rollup/settings/RollupSettings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class RollupSettings {
const val DEFAULT_RENEW_LOCK_RETRY_DELAY = 1000L
const val DEFAULT_CLIENT_REQUEST_RETRY_COUNT = 3
const val DEFAULT_CLIENT_REQUEST_RETRY_DELAY = 1000L
const val MINIMUM_CANCEL_AFTER_TIME_INTERVAL_MINUTES = 10L
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this be a ISM specific cluster settings and default value is 10 minutes? @bowenlan-amzn WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it will be more flexible. @Joshua152 can do this as a follow up.


val ROLLUP_ENABLED: Setting<Boolean> = Setting.boolSetting(
"plugins.rollup.enabled",
Expand Down
Loading