[Extensions] Increasing AD Extension Request timeout to 20 and fixes exception handling for Delete/Profile #916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ST_TIMEOUT setting, increased request timeout setting to 20, fixed affected tests Signed-off-by: Joshua Palis <[email protected]>
joshpalis
requested review from
jmazanec15,
jngz-es,
kaituo,
saratvemulapalli,
ohltyler,
vamshin,
VijayanB,
ylwu-amzn,
amitgalitz,
sudiptoguha,
jackiehanyang,
sean-zheng-amazon,
dbwiddis and
owaiskazi19
as code owners
owaiskazi19
reviewed
May 31, 2023
| @@ -57,7 +57,7 @@ private LegacyOpenDistroAnomalyDetectorSettings() {} | |||
| public static final Setting<TimeValue> REQUEST_TIMEOUT = Setting | |||
| .positiveTimeSetting( | |||
| "opendistro.anomaly_detection.request_timeout", | |||
| TimeValue.timeValueSeconds(10), | |||
| TimeValue.timeValueSeconds(20), | |||
There was a problem hiding this comment.
Can you add a TODO here to revert this back once we have support for cluster state? That's one of major reason for the timeout
Signed-off-by: Joshua Palis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## feature/extensions #916 +/- ##
========================================================
+ Coverage 41.88% 43.36% +1.47%
- Complexity 2335 2435 +100
========================================================
Files 301 301
Lines 18026 18065 +39
Branches 1871 1872 +1
========================================================
+ Hits 7550 7833 +283
+ Misses 9918 9651 -267
- Partials 558 581 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
|
owaiskazi19
approved these changes
May 31, 2023
vibrantvarun
approved these changes
May 31, 2023
…er than type Signed-off-by: Joshua Palis <[email protected]>
joshpalis
changed the title
owaiskazi19
approved these changes
May 31, 2023
dbwiddis
approved these changes
Jun 1, 2023
| @@ -163,6 +167,7 @@ public class CheckpointDao { | |||
| public CheckpointDao( | |||
| SDKRestClient client, | |||
| OpenSearchAsyncClient sdkJavaAsyncClient, | |||
| Settings settings, | |||
There was a problem hiding this comment.
I'm really iffy on adding a new parameter to an already-long list, but if we do so it should probably be at the end of the list, not in the middle. Also we could add an overload that delegates from the previous version to the new version with Settings.EMPTY to avoid having to change every single existing call.
| sdkClusterService, | ||
| indexNameExpressionResolver, | ||
| javaAsyncClient(), | ||
| environmentSettings |
There was a problem hiding this comment.
Similar comment here; we do have the settings at the end but can we just add an overloaded constructor that assumes empty settings if not passed so we don't have to rewrite existing constructor calls?
…ntDao to assume an empty settings object if not passed directly Signed-off-by: Joshua Palis <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removes all hardcoded timeout values for SDKClient/OpenSearchAsyncClinet requests with
AnomalyDetectorSettings.REQUEST_TIMEOUTvalue, increases timeout from 10 seconds to 20.This PR is an effort to unblock REST integration tests, as all REST IT tests are currently failing due to the following timeout error :
Additionally, fixes exception handling for DeleteDetector/ProfileDetector to check exception message rather than type to determine if the exception is due to a missing index
Issues Resolved
Part of opensearch-project/opensearch-sdk-java#724, fixes timeout issues seen in integration testing PR (#913)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.