log the observed status in redshift sensor #29274
Merged
Conversation
This file contains hidden or 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
boring-cyborg
bot
added
area:providers
provider:amazon
vincbeck
approved these changes
Jan 31, 2023
josh-fell
approved these changes
Feb 1, 2023
| return self.hook.cluster_status(self.cluster_identifier) == self.target_status | ||
| current_status = self.hook.cluster_status(self.cluster_identifier) | ||
| self.log.info( | ||
| "Poked cluster %s for status '%s', found status '%s'", |
Contributor
There was a problem hiding this comment.
Suggested change
| "Poked cluster %s for status '%s', found status '%s'", | |
| "Poked cluster %s for status %r, found status %r", |
Contributor
Author
There was a problem hiding this comment.
I'd be happy to apply the suggestion, but can you explain why this would be better ? Both being strings, the output would be exactly the same, wouldn't it ?
Contributor
There was a problem hiding this comment.
Yes. Just merely a suggestion for readability and maybe a little future-proofing with all of those statuses if there is some Enum to be created with a nice repr. Certainly isn't a blocking suggestion to use repr or not.
58 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
this sensor only checks against the "wanted" status, and not other possibly terminal statuses.
So if the cluster takes an unexpected branch and ends up in a semi-terminal state, this is completely invisible, and would only fail with an uninformative timeout after a long time.
The "proper" fix would be to have a list of terminal states to check against, but I don't have enough redshift knowledge to know what's terminal in the long list of possible statuses:
So I'm taking this poor man's approach that'd at least allow us to get a sense of what happened from looking at the logs.
(also, since the sensor is very generic, I think it'd be super hard to do because
storage-fullfor instance could be terminal in some cases, except if we run a step to free space and are waiting on the cluster leaving that state ? So many possibilities...)