Let querier plugin decide to delete state object on error #3182
Conversation
Codecov Report@@ Coverage Diff @@
## master #3182 +/- ##
==========================================
+ Coverage 77.56% 77.56% +<.01%
==========================================
Files 432 433 +1
Lines 22318 22323 +5
==========================================
+ Hits 17309 17313 +4
- Misses 5009 5010 +1
Continue to review full report at Codecov.
|
m4dcoder
force-pushed
the
robust-mistral-querier
branch
from
0b8d565
to
c83a84c
Compare
|
From high level - LGTM. Looking at the code, it doesn't seem to be a way for user / operator to enable this option for Mistral since it's disabled by default. Or am I missing something? Edit: Ignore last part of my comment. It seems like this option is in addition to deleting on completion, etc. - https://github.com/StackStorm/st2/pull/3182/files#diff-5d34721fd4bff559a18008d8c41031ccR121 |
| execution = self._client.executions.get(exec_id) | ||
| except mistralclient_base.APIException as mistral_exc: | ||
| if 'not found' in mistral_exc.message: | ||
| raise exceptions.ReferenceNotFoundError(mistral_exc.message) |
| for task in self._client.tasks.list(workflow_execution_id=exec_id) | ||
| ] | ||
| try: | ||
| wf_tasks = [ |
There was a problem hiding this comment.
I would personally prefer to make this a regular for loop, because list comprehensions are usually only intended to be used for simple transformations and in cases like that when you have exception handling, etc. regular for loop is more clear and obvious.
There was a problem hiding this comment.
Sure can change to a for loop
| if 'not found' in mistral_exc.message: | ||
| raise exceptions.ReferenceNotFoundError(mistral_exc.message) | ||
| raise mistral_exc | ||
| except Exception: |
There was a problem hiding this comment.
This is redundant since we are not doing any handling of the exception here so it can be removed.
There was a problem hiding this comment.
Will it still raise any general exception if I remove it?
| if 'not found' in mistral_exc.message: | ||
| raise exceptions.ReferenceNotFoundError(mistral_exc.message) | ||
| raise mistral_exc | ||
| except Exception: |
| tracker.shutdown() | ||
|
|
||
| # Ensure state objects are deleted | ||
| self.assertRaises( |
There was a problem hiding this comment.
Should we also verify somewhere at the top that this object actually exists in the database in the first place?
|
It's not an option for the user/operator. It's a querier plugin level option. |
Add option in querier plugin to specify whether state object should be deleted if an error occur during query. This allows querier to recover in case of unexpected environment related exceptions. Mistral querier is refactored to delete state object on workflow completion or when the workflow or task references no longer exists.
Refactor mistral querier get tasks function to use a for loop instead of a list comprehension.
Add checks in the querier unit tests to ensure state objects are in the database before tests start.
m4dcoder
force-pushed
the
robust-mistral-querier
branch
from
c83a84c
to
6630a08
Compare
|
@Kami concerns/comments addressed. |
Add option in querier plugin to specify whether state object should be deleted if an error occur during query. This allows querier to recover in case of unexpected environment related exceptions. Mistral querier is refactored to delete state object on workflow completion or when the workflow or task references no longer exists.