Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI feedback of state machine #1880

Merged
merged 1 commit into from
May 21, 2020
Merged

UI feedback of state machine #1880

merged 1 commit into from
May 21, 2020

Conversation

SWvheerden
Copy link
Collaborator

Description

Adds a channel to the base_node parser to listen for state machine publisher to publish its state and info.

Motivation and Context

This PR provides feedback for the user to see what the status and info from the state machine is, like how syncing is, is it syncing etc.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@SWvheerden SWvheerden force-pushed the sw_add_state_UI_feedback branch 2 times, most recently from a56db65 to d5b84a0 Compare May 19, 2020 09:01
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

LGTM - few small changes

applications/tari_base_node/src/parser.rs Outdated Show resolved Hide resolved
self.executor.spawn(async move {
match channel.next().await {
None => {
println!("Something went wrong");
Copy link
Member

Choose a reason for hiding this comment

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

None happens when the channel closes (e.g. if the node shuts down). I wouldn't print any output in this case as it might give the user an impression of instability. The log below could be an info! saying that the state machine state channel closed.

@@ -64,6 +64,7 @@ impl ListeningData {
pub async fn next_event<B: BlockchainBackend>(&mut self, shared: &mut BaseNodeStateMachine<B>) -> StateEvent {
info!(target: LOG_TARGET, "Listening for chain metadata updates");
shared.info = StatusInfo::Listening(ListeningInfo::new());
let _ = shared.status_event_publisher.send(shared.info.clone()).await;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, publishing an event would just be wrapped in a function

Suggested change
let _ = shared.status_event_publisher.send(shared.info.clone()).await;
shared.publish_status_event(shared.info.clone()).await;

@@ -161,6 +161,7 @@ impl BlockSyncStrategy {
) -> StateEvent
{
shared.info = StatusInfo::BlockSync(BlockSyncInfo::new(None, None, None));
let _ = shared.status_event_publisher.send(shared.info.clone()).await;
Copy link
Member

Choose a reason for hiding this comment

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

Write and use a publish_status_event function

@SWvheerden SWvheerden force-pushed the sw_add_state_UI_feedback branch from a610e52 to 90c1c18 Compare May 20, 2020 08:41
sdbondi
sdbondi previously approved these changes May 20, 2020
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

LGTM - we might want to implement a set_state_machine_info function that sets the info and publishes the event to remove the possibility of bugs from not publishing an event. But not NB for now

sdbondi
sdbondi previously approved these changes May 20, 2020
…edback

Changed listing state display to be more clear
Co-authored-by: Stan Bondi <[email protected]>
Add comment to describe why the channel will never be empty
@SWvheerden SWvheerden force-pushed the sw_add_state_UI_feedback branch from 8147ea1 to a6a9cd6 Compare May 20, 2020 15:25
@sdbondi sdbondi merged commit e1b70d2 into development May 21, 2020
@sdbondi sdbondi deleted the sw_add_state_UI_feedback branch May 22, 2020 11:46
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.

2 participants