Skip to content

chore: Add additional logs for state transitions - #989

Merged
darunrs merged 4 commits into
mainfrom
add-additional-logs-for-state-transitions
Aug 9, 2024
Merged

darunrs merged 4 commits into
mainfrom
add-additional-logs-for-state-transitions

Conversation

@darunrs

@darunrs darunrs commented Aug 7, 2024

Copy link
Copy Markdown
Contributor

I added a little bit more logging to the lifecycle state changes, as well as renamed the Stopping/Stopped states to Suspending/Suspended states to more accurately capture the intention.

@darunrs
darunrs requested a review from a team August 7, 2024 18:51
@darunrs

darunrs commented Aug 7, 2024 •

Copy link
Copy Markdown
Contributor Author

Thinking of adding unit tests to lifecycle, where I just test that the handle_[state] functions do what we expect. This requires importing a LOT of mocked things. Not totally sure what's the cleanest way of doing that. I thought of two ways:

  1. Implement an auto mock for all three handler structs and use them
  2. Create actual handlers, with mocked things like redis, wrappers, etc.

I initially thought to do 1 but now I'm starting to feel 2 might be better?

@darunrs darunrs linked an issue Aug 7, 2024 that may be closed by this pull request
Comment thread coordinator/src/indexer_state.rs Outdated
lifecycle_state: if old_state.lifecycle_state == OldLifecycleState::Stopping {
LifecycleState::Suspending
} else {
LifecycleState::Suspended

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would set any state that isn't Stopping to Suspended right? We should only set this if it is Stopped. We probably need to map each state specifically since we have two types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tested this locally, and it actually doesn't reach this part of the function, unless the initial parse fails. So, all the other options ended up getting parsed correctly, and the only ones which went here, were the states which are no longer part of Lifecycle.

I verified this in the below unit test. I have four states to parse: Initializing, Running, Stopping, and Stopped. The redis state set call was only called twice, and they were called with the correct Suspending and Suspended state respectively.

I don't mind being explicit here though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right, we'd only get here if parsing fails, which would only be for Stopping/Stopped. I'm fine leaving it as is then :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the if statement to a separate variable assignment. I'll be able to put some warn logs out if there's an unexpected input.

@darunrs
darunrs merged commit 4af58bc into main Aug 9, 2024
@darunrs
darunrs deleted the add-additional-logs-for-state-transitions branch August 9, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add logs for all state transitions

2 participants