Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix for #90 - Add stats to management api #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maniankara
Copy link
Contributor

Also added tests.

@hjr3 hjr3 self-requested a review June 28, 2017 19:08
Copy link
Owner

@hjr3 hjr3 left a comment

Choose a reason for hiding this comment

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

This compiles, but does not work the way we want. The design of weldr is that the manager process receives API requests. However, the worker processes are the ones who track stats. We need to have the manager periodically ask the workers how many successes and errors they have via the capnp setup I put into place.

When asking the workers, we have to consider what happens if a worker process gets restarted. In that case, the worker will start the stats over from 0, but the manager will want to accumulate the stats over time. Thus, I think we should have the worker send over the success and failure counts and then zero out the numbers. The manager would then add the counts each time it asks the workers for the counts. What do you think?

@@ -142,6 +142,8 @@ impl Backend {
pub fn mark_down(&self) {
self.inner.borrow_mut().state = ServerState::Down;
}

pub fn server_stats(&self) -> Stats { self.inner.borrow_mut().stats.clone()}
Copy link
Owner

Choose a reason for hiding this comment

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

This does not need to be borrow_mut().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, actually it doesnt have to be a mut borrow. I will check and correct.

@hjr3
Copy link
Owner

hjr3 commented Jun 28, 2017

Thank you for working on this. I was traveling so it took me a few days to get back to you. I just saw your questions on issue #90 as well.

There is a lot of copy/paste on the API part. I do not think it is ideal, but I also do not see a better abstraction yet.

@maniankara
Copy link
Contributor Author

@hjr3 Alright! Very true, the idea between the workers and managers. I will try to re-implement something and get back.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants