Throw errors on model deletion failures #834

jmazanec15 · 2023-03-31T22:04:13Z

Description

Throws errors on model deletion. Previously we were indicating failure, but returning 200 response codes. This changes that to throw exceptions with the correct response code.

Deprecates constructor for DeleteModelResponse that takes an error/result, as this result should only be returned on success.

Issues Resolved

#831

Check List

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.

src/main/java/org/opensearch/knn/common/exception/DeleteModelWhenInTrainStateException.java

navneet1v · 2023-04-03T18:19:51Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

+                Exception e = new ResourceNotFoundException(errorMessage);
+                logger.error(e);


This is really interesting use of exception. In general we log the exception trace so that we know from where exception came. but here this is not the case.

If we put a log.error message here will that not be sufficient?

We also need exception instance to pass it to the listener.onFailure, I guess that's the main reason why we create it

In general we log the exception trace so that we know from where exception came. but here this is not the case.

Im not sure what you mean by this. I wanted to log the error here so that we can see the complete stack trace. We can also just log the errorMessage, but I felt adding additional information about the exception would be helpful.

What information you are adding here? I see log can do the job.

Im not sure what you mean by this.

What I mean was why not log the error message in when listener.onFailure(e); is executed, which is the line just below this code.

Got it, I updated.

navneet1v · 2023-04-03T18:21:18Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

+                    String errorMessage = String.format("Cannot delete model [%s]. Model is still in training", modelId);
+                    Exception e = new DeleteModelWhenInTrainStateException(errorMessage);
+                    logger.error(e);


same as above.

Will track discussion above.

naveentatikonda · 2023-04-03T18:30:41Z

Do you also want to refactor the BWC tests for model Deletion?
https://github.com/opensearch-project/k-NN/blob/main/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/ModelIT.java

jmazanec15 · 2023-04-04T18:46:12Z

@naveentatikonda yes, I missed that one. Will update.

codecov-commenter · 2023-04-04T20:06:48Z

Codecov Report

Merging #834 (02f7129) into main (1b6fd48) will decrease coverage by 0.14%.
The diff coverage is 79.16%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #834      +/-   ##
============================================
- Coverage     85.42%   85.28%   -0.14%     
- Complexity     1091     1092       +1     
============================================
  Files           152      153       +1     
  Lines          4404     4417      +13     
  Branches        393      392       -1     
============================================
+ Hits           3762     3767       +5     
- Misses          465      473       +8     
  Partials        177      177

Impacted Files	Coverage Δ
...main/java/org/opensearch/knn/indices/ModelDao.java	`80.87% <68.75%> (-0.69%)`	⬇️
...xception/DeleteModelWhenInTrainStateException.java	`100.00% <100.00%> (ø)`
...arch/knn/plugin/transport/DeleteModelResponse.java	`81.48% <100.00%> (-18.52%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Throws errors on model deletion. Previously we were indicating failure, but returning 200 response codes. This changes that to throw exceptions with the correct response code. Signed-off-by: John Mazanec <[email protected]>

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 · 2023-04-10T23:26:47Z

Failure on windows related to: opensearch-project/OpenSearch#7080.

Please review @naveentatikonda @martin-gaievski @navneet1v

Throws errors on model deletion. Previously we were indicating failure, but returning 200 response codes. This changes that to throw exceptions with the correct response code. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 5c3bf53)

Throws errors on model deletion. Previously we were indicating failure, but returning 200 response codes. This changes that to throw exceptions with the correct response code. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 5c3bf53) Co-authored-by: John Mazanec <[email protected]>

jmazanec15 added the Bug Fixes Changes to a system or product designed to handle a programming bug/glitch label Mar 31, 2023

jmazanec15 requested a review from a team March 31, 2023 22:04

jmazanec15 added the backport 2.x label Mar 31, 2023

naveentatikonda reviewed Apr 3, 2023

View reviewed changes

src/main/java/org/opensearch/knn/common/exception/DeleteModelWhenInTrainStateException.java Show resolved Hide resolved

navneet1v reviewed Apr 3, 2023

View reviewed changes

jmazanec15 requested review from navneet1v, naveentatikonda and martin-gaievski April 4, 2023 18:53

jmazanec15 force-pushed the issue-831 branch from 7b00ec7 to 87367cb Compare April 4, 2023 20:03

jmazanec15 force-pushed the issue-831 branch 3 times, most recently from cd6d8b0 to 6b20a9d Compare April 6, 2023 17:39

jmazanec15 added 7 commits April 10, 2023 11:28

Throw errors on model deletion failures

0125dcf

Throws errors on model deletion. Previously we were indicating failure, but returning 200 response codes. This changes that to throw exceptions with the correct response code. Signed-off-by: John Mazanec <[email protected]>

Update changelog

9c36e31

Signed-off-by: John Mazanec <[email protected]>

Fix bwc model test

71394f0

Signed-off-by: John Mazanec <[email protected]>

Update exception comments

165f400

Signed-off-by: John Mazanec <[email protected]>

Fix delete training model cleanup in bwc

14151de

Signed-off-by: John Mazanec <[email protected]>

Fix rebase related errors

a0f3c0e

Signed-off-by: John Mazanec <[email protected]>

Fix corner case string format bug

02f7129

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the issue-831 branch from 7ecb036 to 02f7129 Compare April 10, 2023 18:28

Move error logger to listener

a730725

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the issue-831 branch from 7f435c9 to a730725 Compare April 10, 2023 21:08

naveentatikonda approved these changes Apr 11, 2023

View reviewed changes

navneet1v approved these changes Apr 13, 2023

View reviewed changes

jmazanec15 merged commit 5c3bf53 into opensearch-project:main Apr 13, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.x] Throw errors on model deletion failures #855

Merged

naveentatikonda added the backport 2.7 label Apr 13, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.7] Throw errors on model deletion failures #856

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw errors on model deletion failures #834

Throw errors on model deletion failures #834

jmazanec15 commented Mar 31, 2023

navneet1v Apr 3, 2023

martin-gaievski Apr 3, 2023

jmazanec15 Apr 4, 2023

navneet1v Apr 4, 2023

jmazanec15 Apr 10, 2023

navneet1v Apr 3, 2023

jmazanec15 Apr 4, 2023

naveentatikonda commented Apr 3, 2023

jmazanec15 commented Apr 4, 2023

codecov-commenter commented Apr 4, 2023 •

edited

Loading

jmazanec15 commented Apr 10, 2023

		Exception e = new ResourceNotFoundException(errorMessage);
		logger.error(e);

Throw errors on model deletion failures #834

Throw errors on model deletion failures #834

Conversation

jmazanec15 commented Mar 31, 2023

Description

Issues Resolved

Check List

navneet1v Apr 3, 2023

Choose a reason for hiding this comment

martin-gaievski Apr 3, 2023

Choose a reason for hiding this comment

jmazanec15 Apr 4, 2023

Choose a reason for hiding this comment

navneet1v Apr 4, 2023

Choose a reason for hiding this comment

jmazanec15 Apr 10, 2023

Choose a reason for hiding this comment

navneet1v Apr 3, 2023

Choose a reason for hiding this comment

jmazanec15 Apr 4, 2023

Choose a reason for hiding this comment

naveentatikonda commented Apr 3, 2023

jmazanec15 commented Apr 4, 2023

codecov-commenter commented Apr 4, 2023 • edited Loading

Codecov Report

jmazanec15 commented Apr 10, 2023

codecov-commenter commented Apr 4, 2023 •

edited

Loading