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

Add compile-time option TRACK_STANDBY_VOTES #987 #1140

Closed
wants to merge 1 commit into from

Conversation

abitmore
Copy link
Member

PR for #987.

Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Just to make sure I am understanding the issue correctly:

This will help the API nodes get correct tallies of votes for inactive objects without slowing down other nodes that don't care about vote counts for inactive objects.

The code looks great. I plan to take a deeper dive soon.

Tests would be nice, but testing compile switches is a pain and near impossible to do in an automated way.

Just a note here so we don't forget: Documentation of this switch should be made available.

jmjatlanta
jmjatlanta previously approved these changes Jul 25, 2018
Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Tested with Ubuntu 18.04 (Boost 1.65/OpenSSL 1.1) with option STANDBY_VOTES_TRACKING ON as well as OFF. Everything worked as expected.

A unit test would be nice, even if it only checked the expected results depending on the currently compiled option.

@oxarbitrage
Copy link
Member

oxarbitrage commented Jul 25, 2018

@jmjatlanta we need to merge this one before: #1099
#1099 one haves a test case: #1169

we should do the same here.
do you have time to make it ?

the flow to merge the last @abitmore pull requests can be:

@jmjatlanta
Copy link
Contributor

The test case in #1169 is a great template for this one (and 1099 as well).

I will create the tests in different but similarly named files to avoid merge conflicts. We can merge them into 1 file later.

I will probably need help verifying that I'm creating the environment that tests the actual change.

@oxarbitrage
Copy link
Member

as discussed in telegram; @jmjatlanta will review #1099 and #1169 and we can merge them those if they are ready.

@oxarbitrage will do the test case for #1140

@oxarbitrage oxarbitrage mentioned this pull request Jul 25, 2018
1 task
@pmconrad
Copy link
Contributor

Tests would be nice, but testing compile switches is a pain and near impossible to do in an automated way.

Good point. How about making it a runtime config switch instead. We already have a configurable setting in database, namely the checkpoints list, so I suppose adding more options is not an architectural breach. Opinions?

@abitmore
Copy link
Member Author

Good point. How about making it a runtime config switch instead. We already have a configurable setting in database, namely the checkpoints list, so I suppose adding more options is not an architectural breach. Opinions?

Will do. Actually run-time option is required in the user story.

@abitmore
Copy link
Member Author

Replaced by #1191

@abitmore abitmore closed this Jul 27, 2018
@abitmore abitmore deleted the 987-standby-votes branch July 30, 2018 19:45
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.

4 participants