Add retry mechanism for neural search inference. #91

zane-neo · 2022-12-27T02:57:04Z

Description

This feature is an enhancement to neural-search and the purpose is to improve the availability of the opensearch ml cluster. The core idea is add retry for inference operation, once a request is been sending or have sent to a ml instance which is not available now, rather than simply fail this request we use retry to improve the success rate of requests.

Issues Resolved

#92

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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.

Signed-off-by: Zan Niu <[email protected]>

navneet1v · 2022-12-27T03:04:18Z

src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java

+    private void inferenceSentencesWithRetry(
+        @NonNull final List<String> targetResponseFilters,
+        @NonNull final String modelId,
+        @NonNull final List<String> inputText,
+        final int retryTime,
+        @NonNull final ActionListener<List<List<Float>>> listener


remove @nonnull from private functions. Given that it is private function we can be sure that the inputs shouldn't be null.

navneet1v · 2022-12-27T03:06:54Z

src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java

+            final int nodeNotConnectedExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeNotConnectedException.class);
+            final int nodeDisconnectExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeDisconnectedException.class);
+            if ((nodeDisconnectExceptionIndex != -1 || nodeNotConnectedExceptionIndex != -1) && retryTime < MAX_RETRY) {


We should encapsulate this in a different method which can give a boolean response if we should do a retry or not. This will allow us in future to add more retryable exceptions and will also allow us to evolve the conditions for doing the retry.

Refactored to a new method.

navneet1v · 2022-12-27T03:07:59Z

src/test/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessorTests.java

@@ -114,6 +119,28 @@ public void testInferenceSentences_whenExceptionFromMLClient_thenFailure() {
        Mockito.verifyNoMoreInteractions(resultListener);
    }

+    public void testInferenceSentences_whenNodeNotConnectedException_thenRetry() {


Add these test cases also:

Test case which tests when all the retries are finished.

Test cases which test when these retryable exceptions are not thrown we should get a failure.

Two cases covered:

check the method testInferenceSentences_whenNodeNotConnectedException_thenRetry_3Times and the Mockito.verify(client, times(4)) stands for the client method has been invoked for 4 times in total.

Add a new method testInferenceSentences_whenNotConnectionException_thenNoRetry and the verify has time of 1, meaning method only run once.

Signed-off-by: Zan Niu <[email protected]>

navneet1v · 2022-12-28T00:00:58Z

src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java

+    private boolean shouldRetry(Exception e, int retryTime) {
+        final int nodeNotConnectedExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeNotConnectedException.class);
+        final int nodeDisconnectExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeDisconnectedException.class);
+        return (nodeDisconnectExceptionIndex != -1 || nodeNotConnectedExceptionIndex != -1) && retryTime < MAX_RETRY;


I would suggest 1 more simplification here. It will allow us just to add new Exceptions in future to the list doing the retry.

Suggested change

private boolean shouldRetry(Exception e, int retryTime) {

final int nodeNotConnectedExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeNotConnectedException.class);

final int nodeDisconnectExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeDisconnectedException.class);

return (nodeDisconnectExceptionIndex != -1 || nodeNotConnectedExceptionIndex != -1) && retryTime < MAX_RETRY;

private boolean shouldRetry(Exception e, int retryTime) {

return (RetryUtil.containsException(INFERENCE_API_RETRYABLE_EXCEPTION_LIST, e)) && retryTime < MAX_RETRY;

}

class RetryUtil {

public static boolean containsException(@NonNull final List<Class<?>> exceptionClassesList, final Exception ex) {

// call below function to check if any exception is present and then return true.

}

private static boolean containsException(final Exception ex, final ClassClass<?> exceptionClazz) {

return ExceptionUtils.indexOfThrowable(ex, exceptionClazz) != -1;

}

}

In fact, we have analyzed all the exceptions if they're retryable and it doesn't seem to have more exceptions could be retried, otherwise the retry may cause cluster level instability, I wouldn't suggest go too far and over design for this for now.

@zane-neo This logic would retry if a model is not loaded, correct?

In fact, we have analyzed all the exceptions if they're retryable and it doesn't seem to have more exceptions could be retried, otherwise the retry may cause cluster level instability, I wouldn't suggest go too far and over design for this for now.

So you are saying there will be no case in future where we need add exception? Thats very strange. I would recommend lets build retry mechanism extensible.

Also the suggested code is just an example.

@zane-neo This logic would retry if a model is not loaded, correct?

No, this logic only retries the network issue: NodeNotConnectedException and NodeDisconnectedException. When a model is not loaded the requests won't be dispatched to that node at the beginning.

In fact, we have analyzed all the exceptions if they're retryable and it doesn't seem to have more exceptions could be retried, otherwise the retry may cause cluster level instability, I wouldn't suggest go too far and over design for this for now.

So you are saying there will be no case in future where we need add exception? Thats very strange. I would recommend lets build retry mechanism extensible.

Also the suggested code is just an example.

Are you talking about business level retries? These should be retried at underlying layers to avoid network calls. But I can do the refactor to the potential business retries at client side.

Are you talking about business level retries?

No.

I am not saying to refractor any code at client side. I am saying just refractor the code at the Neural Search plugin side as suggested in my older comment.

jmazanec15

Retries would be useful for ephemeral issues, correct? So which exceptions would we expect to be resolved on failure like this?

Signed-off-by: Zan Niu <[email protected]>

zane-neo · 2023-01-04T01:13:48Z

Retries would be useful for ephemeral issues, correct? So which exceptions would we expect to be resolved on failure like this?

Two exceptions we're going to handle are: NodeNotConnectedException and NodeDisconnectedException

Signed-off-by: Zan Niu <[email protected]>

Add basic retry mechanism for neural search inference Signed-off-by: Zan Niu <[email protected]> (cherry picked from commit b207176)

Add basic retry mechanism for neural search inference (cherry picked from commit b207176) Signed-off-by: John Mazanec <[email protected]>

Add basic retry mechanism for neural search inference Signed-off-by: Zan Niu <[email protected]>

zane-neo added 2 commits December 27, 2022 10:48

Add basic retry machenism for neural search inference

0d05451

Signed-off-by: Zan Niu <[email protected]>

Add basic retry machenism for neural search inference

24c7576

Signed-off-by: Zan Niu <[email protected]>

zane-neo requested a review from a team December 27, 2022 02:57

zane-neo changed the title ~~Neural search retry~~ Add retry mechanism for neural search inference. Dec 27, 2022

zane-neo requested review from model-collapse and jmazanec15 December 27, 2022 02:59

navneet1v reviewed Dec 27, 2022

View reviewed changes

Add basic retry mechanism for neural search inferrence

a8d5926

Signed-off-by: Zan Niu <[email protected]>

navneet1v reviewed Dec 28, 2022

View reviewed changes

jmazanec15 reviewed Dec 30, 2022

View reviewed changes

zane-neo added 2 commits January 3, 2023 15:56

Add retry mechanism for ml client

1c33045

Signed-off-by: Zan Niu <[email protected]>

Add retry mechanism for ml client

4f1e3d3

Signed-off-by: Zan Niu <[email protected]>

Add basic retry machenism for neural search inference

a87dd70

Signed-off-by: Zan Niu <[email protected]>

jmazanec15 approved these changes Jan 6, 2023

View reviewed changes

navneet1v approved these changes Jan 10, 2023

View reviewed changes

jmazanec15 added Enhancements Increases software capabilities beyond original client specifications backport 2.x Label will add auto workflow to backport PR to 2.x branch backport 2.5 labels Jan 10, 2023

jmazanec15 merged commit b207176 into opensearch-project:main Jan 10, 2023

opensearch-trigger-bot bot mentioned this pull request Jan 10, 2023

[Backport 2.x] Add retry mechanism for neural search inference. #98

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 10, 2023

Add retry mechanism for neural search inference (#91)

2642f40

Add basic retry mechanism for neural search inference Signed-off-by: Zan Niu <[email protected]> (cherry picked from commit b207176)

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 10, 2023

Add retry mechanism for neural search inference (#91)

97f587e

Add basic retry mechanism for neural search inference Signed-off-by: Zan Niu <[email protected]> (cherry picked from commit b207176)

opensearch-trigger-bot bot mentioned this pull request Jan 10, 2023

[Backport 2.5] Add retry mechanism for neural search inference. #99

Merged

jmazanec15 pushed a commit that referenced this pull request Jan 10, 2023

Add retry mechanism for neural search inference (#91)

c951aa0

Add basic retry mechanism for neural search inference (cherry picked from commit b207176) Signed-off-by: John Mazanec <[email protected]>

jmazanec15 pushed a commit that referenced this pull request Jan 10, 2023

Add retry mechanism for neural search inference (#91)

f406c6a

Add basic retry mechanism for neural search inference (cherry picked from commit b207176) Signed-off-by: John Mazanec <[email protected]>

navneet1v pushed a commit to navneet1v/neural-search that referenced this pull request Feb 24, 2023

Add retry mechanism for neural search inference (opensearch-project#91)

46803c3

Add basic retry mechanism for neural search inference Signed-off-by: Zan Niu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry mechanism for neural search inference. #91

Add retry mechanism for neural search inference. #91

zane-neo commented Dec 27, 2022 •

edited

Loading

navneet1v Dec 27, 2022

zane-neo Dec 27, 2022

navneet1v Dec 27, 2022

zane-neo Dec 27, 2022

navneet1v Dec 27, 2022

zane-neo Dec 27, 2022

navneet1v Dec 28, 2022 •

edited

Loading

zane-neo Dec 28, 2022

jmazanec15 Dec 30, 2022

navneet1v Jan 1, 2023

zane-neo Jan 3, 2023

zane-neo Jan 3, 2023

navneet1v Jan 3, 2023

jmazanec15 left a comment

zane-neo commented Jan 4, 2023

-    private boolean shouldRetry(Exception e, int retryTime) {
-        final int nodeNotConnectedExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeNotConnectedException.class);
-        final int nodeDisconnectExceptionIndex = ExceptionUtils.indexOfThrowable(e, NodeDisconnectedException.class);
-        return (nodeDisconnectExceptionIndex != -1 || nodeNotConnectedExceptionIndex != -1) && retryTime < MAX_RETRY;
+    private boolean shouldRetry(Exception e, int retryTime) {
+        return (RetryUtil.containsException(INFERENCE_API_RETRYABLE_EXCEPTION_LIST, e)) && retryTime < MAX_RETRY;
+}
+class RetryUtil {
+    public static boolean containsException(@NonNull final List<Class<?>> exceptionClassesList, final Exception ex) {
+     // call below function to check if any exception is present and then return true.
+    }
+    private static boolean containsException(final Exception ex, final ClassClass<?> exceptionClazz) {
+         return ExceptionUtils.indexOfThrowable(ex, exceptionClazz) != -1;
+    }
+}

Add retry mechanism for neural search inference. #91

Add retry mechanism for neural search inference. #91

Conversation

zane-neo commented Dec 27, 2022 • edited Loading

Description

Issues Resolved

Check List

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

navneet1v Dec 28, 2022 • edited Loading

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

jmazanec15 left a comment

Choose a reason for hiding this comment

zane-neo commented Jan 4, 2023

zane-neo commented Dec 27, 2022 •

edited

Loading

navneet1v Dec 28, 2022 •

edited

Loading