Skip to content

Commit

Permalink
Revert inadvertent migration of deleteController to SdkClient (#2961)
Browse files Browse the repository at this point in the history 
Signed-off-by: Daniel Widdis <[email protected]>
  • Loading branch information
@dbwiddis
dbwiddis committed Sep 17, 2024
1 parent b26d8b4 commit 66d8e2b
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 48 deletions.
73 changes: 34 additions & 39 deletions plugin/src/main/java/org/opensearch/ml/action/models/DeleteModelTransportAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.ml.action.models;

import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
import static org.opensearch.common.xcontent.json.JsonXContent.jsonXContent;
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.opensearch.ml.common.CommonValue.ML_CONTROLLER_INDEX;
Expand All @@ -25,12 +26,12 @@
import org.opensearch.OpenSearchStatusException;
import org.opensearch.ResourceNotFoundException;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.delete.DeleteRequest;
import org.opensearch.action.delete.DeleteResponse;
import org.opensearch.action.get.GetResponse;
import org.opensearch.action.support.ActionFilters;
import org.opensearch.action.support.HandledTransportAction;
import org.opensearch.client.Client;
import org.opensearch.client.opensearch._types.OpenSearchException;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
import org.opensearch.common.settings.Settings;
Expand Down Expand Up @@ -383,46 +384,40 @@ private void deleteModelChunksAndController(
* @param modelId model ID
*/
private void deleteController(String modelId, Boolean isHidden, ActionListener<Boolean> actionListener) {
DeleteDataObjectRequest deleteDataObjectRequest = DeleteDataObjectRequest.builder().index(ML_CONTROLLER_INDEX).id(modelId).build();
sdkClient
.deleteDataObjectAsync(deleteDataObjectRequest, client.threadPool().executor(GENERAL_THREAD_POOL))
.whenComplete((r, throwable) -> {
if (throwable == null) {
try {
DeleteResponse deleteResponse = DeleteResponse.fromXContent(r.parser());
log
.info(
getErrorMessage(
"Model controller for the provided model successfully deleted from index, result: {}.",
modelId,
isHidden
),
deleteResponse.getResult()
);
actionListener.onResponse(true);
} catch (Exception e) {
actionListener.onFailure(e);
}
DeleteRequest deleteRequest = new DeleteRequest(ML_CONTROLLER_INDEX, modelId).setRefreshPolicy(IMMEDIATE);
client.delete(deleteRequest, new ActionListener<>() {
@Override
public void onResponse(DeleteResponse deleteResponse) {
log
.info(
getErrorMessage(
"Model controller for the provided model successfully deleted from index, result: {}.",
modelId,
isHidden
),
deleteResponse.getResult()
);
actionListener.onResponse(true);
}

@Override
public void onFailure(Exception e) {
if (e instanceof ResourceNotFoundException) {
log
.info(
getErrorMessage(
"Model controller not deleted due to no model controller found for the given model.",
modelId,
isHidden
)
);
actionListener.onResponse(true); // we consider this as success
} else {
Exception e = SdkClientUtils.unwrapAndConvertToException(throwable);
if (e instanceof ResourceNotFoundException // Local client
|| e instanceof OpenSearchException && // Remote client
((OpenSearchException) e).status() == RestStatus.NOT_FOUND.getStatus()) {
log
.info(
getErrorMessage(
"Model controller not deleted due to no model controller found for the given model.",
modelId,
isHidden
)
);
actionListener.onResponse(true); // we consider this as success
} else {
log.error(getErrorMessage("Failed to delete model controller for the given model.", modelId, isHidden), e);
actionListener.onFailure(e);
}
log.error(getErrorMessage("Failed to delete model controller for the given model.", modelId, isHidden), e);
actionListener.onFailure(e);
}
});
}
});
}

private Boolean isModelNotDeployed(MLModelState mlModelState) {
Expand Down
79 changes: 70 additions & 9 deletions plugin/src/test/java/org/opensearch/ml/action/models/DeleteModelTransportActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,13 @@ public static void cleanup() {
}

public void testDeleteModel_Success() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
Expand Down Expand Up @@ -207,6 +214,13 @@ public void testDeleteModel_Success() throws IOException, InterruptedException {
}

public void testDeleteRemoteModel_Success() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
Expand All @@ -228,11 +242,16 @@ public void testDeleteRemoteModel_Success() throws IOException, InterruptedExcep
}

public void testDeleteRemoteModel_deleteModelController_failed() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onFailure(new RuntimeException("runtime exception"));
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
PlainActionFuture<DeleteResponse> failFuture = PlainActionFuture.newFuture();
failFuture.onFailure(new RuntimeException("runtime exception"));
when(client.delete(any())).thenReturn(future).thenReturn(failFuture);
when(client.delete(any())).thenReturn(future);

doAnswer(invocation -> {
ActionListener<BulkByScrollResponse> listener = invocation.getArgument(2);
Expand All @@ -257,11 +276,16 @@ public void testDeleteRemoteModel_deleteModelController_failed() throws IOExcept
}

public void testDeleteLocalModel_deleteModelController_failed() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onFailure(new RuntimeException("runtime exception"));
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
PlainActionFuture<DeleteResponse> failFuture = PlainActionFuture.newFuture();
failFuture.onFailure(new RuntimeException("runtime exception"));
when(client.delete(any())).thenReturn(future).thenReturn(failFuture);
when(client.delete(any())).thenReturn(future);

doAnswer(invocation -> {
ActionListener<BulkByScrollResponse> listener = invocation.getArgument(2);
Expand All @@ -286,6 +310,13 @@ public void testDeleteLocalModel_deleteModelController_failed() throws IOExcepti
}

public void testDeleteRemoteModel_deleteModelChunks_failed() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
Expand All @@ -312,6 +343,13 @@ public void testDeleteRemoteModel_deleteModelChunks_failed() throws IOException,
}

public void testDeleteHiddenModel_Success() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
Expand Down Expand Up @@ -371,6 +409,13 @@ public void testDeleteHiddenModel_NoSuperAdminPermission() throws IOException, I
}

public void testDeleteModel_Success_AlgorithmNotNull() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
Expand Down Expand Up @@ -455,6 +500,13 @@ public void testDeleteModel_ModelNotFoundException() throws IOException, Interru
}

public void testDeleteModel_deleteModelController_ResourceNotFoundException() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onFailure(new ResourceNotFoundException("errorMessage"));
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
PlainActionFuture<DeleteResponse> failFuture = PlainActionFuture.newFuture();
Expand Down Expand Up @@ -505,6 +557,13 @@ public void test_ValidationFailedException() throws IOException, InterruptedExce
}

public void testDeleteRemoteModel_modelNotFound_ResourceNotFoundException() throws IOException, InterruptedException {
// For controller
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());
// For model
PlainActionFuture<DeleteResponse> failFuture = PlainActionFuture.newFuture();
failFuture.onFailure(new ResourceNotFoundException("resource not found"));
PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
Expand Down Expand Up @@ -567,9 +626,11 @@ public void testModelNotFound_modelChunks_modelController_delete_success() throw
getFuture.onResponse(null);
when(client.get(any())).thenReturn(getFuture);

PlainActionFuture<DeleteResponse> future = PlainActionFuture.newFuture();
future.onResponse(deleteResponse);
when(client.delete(any())).thenReturn(future);
doAnswer(invocation -> {
ActionListener<DeleteResponse> listener = invocation.getArgument(1);
listener.onResponse(deleteResponse);
return null;
}).when(client).delete(any(), any());

doAnswer(invocation -> {
ActionListener<BulkByScrollResponse> listener = invocation.getArgument(2);
Expand Down

0 comments on commit 66d8e2b

Please sign in to comment.