-
Notifications
You must be signed in to change notification settings - Fork 787
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Election age statistics #4537
Election age statistics #4537
Conversation
auto election_start = election->get_election_start (); | ||
auto age = now - election_start; | ||
total_age += age; | ||
if (election_start < oldest_election_start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can oldest_election_start = std::min(oldest_election_start, election->election_start) be used instead of branching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I can do that if using the getter function. That would probably require election_start to be public which I think is a bad idea. It shouldn't be modified outside the election class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be compatible with the getter:
oldest_election_start = std::min(oldest_election_start, election->get_election_start())
node_config.ipc_config.transport_tcp.enabled = true; | ||
node_config.ipc_config.transport_tcp.port = system.get_available_port (); | ||
nano::node_flags node_flags; | ||
node_flags.disable_request_loop = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to explain why you are disabling the request loop.
.work (*system.work.generate (nano::dev::genesis->hash ())) | ||
.build (); | ||
node1->process_active (send1); | ||
ASSERT_TIMELY (5s, !node1->active.empty ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks strange to me. Maybe there is a race condition between process_active() and start_elections(). The usual way to use start_elections() is:
ASSERT_TRUE (nano::test::process (*node1, {send1}));
ASSERT_TRUE (nano::test::start_elections (system, *node1, {send1}));
ASSERT_EQ ("0", response.get<std::string> ("optimistic")); | ||
ASSERT_EQ ("1", response.get<std::string> ("total")); | ||
ASSERT_NE ("0.00", response.get<std::string> ("aec_utilization_percentage")); | ||
ASSERT_NO_THROW (response.get<std::string> ("max_election_age")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 checks could check that the values are within an expected range.
This PR adds information to the
election_statistics
RPC call about the maximum and average age of elections in AEC .This will help identify long running elections.
I have also updated the unit test to create an election
Request:
Response example: