NC-1361 Added configurable refresh delay for SyncingSubscriptionService on start up #383
Conversation
Shinabyss
changed the title
| } | ||
|
|
||
| public SubscriptionManager() { | ||
| this.refreshDelay = 5000; |
There was a problem hiding this comment.
use the const here rather than the number
| + "default: ${DEFAULT-VALUE}", | ||
| defaultValue = "5000" | ||
| ) | ||
| private void setRefreshDelay(final Long refreshDelay) { |
There was a problem hiding this comment.
what happens if I specify 1.5 ? Why long not int?
There was a problem hiding this comment.
vertx handles the sync function after we pass it in a value and it requires a long.
If 1.5 is entered, the CLI will pick it up as an error and warn user because only positive int below 3600000 is accepted.
long datatype are integers, they cannot take in decimals unlike double precision numbers
| defaultValue = "5000" | ||
| ) | ||
| private void setRefreshDelay(final Long refreshDelay) { | ||
| if (refreshDelay < 1 || refreshDelay > 3600000) { |
There was a problem hiding this comment.
Why the magic numbers?
Suggestion: refactor the values into static constants with meaningful name
There was a problem hiding this comment.
5000 milliseconds is the original default when we still had it hard coded. I wanted to keep the configuration the same as the original if user started pantheon without specifying a refresh delay. Although why we decided on 5 seconds to begin with when we had it hard coded is up for debate. Do you have a better default value you would like to share?
There was a problem hiding this comment.
That's not what I mean when I say 'magic numbers' i.e. https://en.wikipedia.org/wiki/Magic_number_%28programming%29
| throw new ParameterException( | ||
| new CommandLine(this), | ||
| "refreshDelay must be a positive integer smaller than 3600000 (1 hour)"); | ||
| } |
There was a problem hiding this comment.
recommend using MAX const in the string message here too
PR description
Added configurable refresh delay for SyncingSubscriptionService on start up allowing user to change how often user/subscribers will sync with the node on the current state.
Config has a boundary of greater than 0 and less than 3600000 (equates to 1 hour) milliseconds.
Added Unit Testing
Not suitable for acceptance testing as acceptance testing this feature require testing the time elapse of the syncing subscriptions. This is performed by vertx hence the function is tested by vertx and secondly and more importantly, attempt to assert time delay as a form of testing can result in brittle test. For instance, setting the refresh delay to 5 seconds and run test could produce measurements on delay between 0-5 seconds and does not provide conclusive test that the config is actually set to 5 seconds. A loop can be used to retest this over and over to produce a more reliable conclusion, but this is resource and time consuming on top of still only provide a higher percentage confidence the config is correctly set and not a conclusive one. And thus as per discussion with other members of product dev team.
Fixed Issue(s)