-
Notifications
You must be signed in to change notification settings - Fork 130
NC-1361 Added configurable refresh delay for SyncingSubscriptionService on start up #383
Changes from 11 commits
334ced0
7b52fd4
4acf4cf
0d517ba
1db0792
189b25a
462536b
3295b21
609cf0c
ae5e2ce
7b90fea
931af28
113ba99
f8dcba8
35d3e7d
00497f2
d67806d
79c09e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -312,6 +312,26 @@ public static class RpcApisConversionException extends Exception { | |
) | ||
private final Collection<RpcApi> wsApis = null; | ||
|
||
private Long refreshDelay; | ||
|
||
@Option( | ||
names = {"--ws-refresh-delay"}, | ||
paramLabel = "<refresh delay>", | ||
arity = "1", | ||
description = | ||
"Refresh delay of websocket subscription sync in milliseconds. " | ||
+ "default: ${DEFAULT-VALUE}", | ||
defaultValue = "5000" | ||
) | ||
private void setRefreshDelay(final Long refreshDelay) { | ||
if (refreshDelay < 1 || refreshDelay > 3600000) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the magic numbers? Suggestion: refactor the values into static constants with meaningful name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. recommend using MAX const in the string message here too |
||
this.refreshDelay = refreshDelay; | ||
} | ||
|
||
@Option( | ||
names = {"--host-whitelist"}, | ||
paramLabel = "<hostname>", | ||
|
@@ -504,6 +524,7 @@ private WebSocketConfiguration webSocketConfiguration() { | |
webSocketConfiguration.setHost(wsHostAndPort.getHost()); | ||
webSocketConfiguration.setPort(wsHostAndPort.getPort()); | ||
webSocketConfiguration.setRpcApis(wsApis); | ||
webSocketConfiguration.setRefreshDelay(refreshDelay); | ||
return webSocketConfiguration; | ||
} | ||
|
||
|
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.
what happens if I specify 1.5 ? Why long not int?
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.
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