Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

NC-1361 Added configurable refresh delay for SyncingSubscriptionService on start up #383

Merged
merged 18 commits into from
Dec 12, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
334ced0
WS sync subscription delay added
Shinabyss Dec 4, 2018
7b52fd4
Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …
Shinabyss Dec 4, 2018
4acf4cf
WS sync subscription delay added with unit testing
Shinabyss Dec 9, 2018
0d517ba
Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …
Shinabyss Dec 9, 2018
1db0792
WS sync subscription delay added with unit testing
Shinabyss Dec 9, 2018
189b25a
Merge branch 'master' into NC-1361
Shinabyss Dec 10, 2018
462536b
Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …
Shinabyss Dec 10, 2018
3295b21
changed number to a constant in constructor
Shinabyss Dec 10, 2018
609cf0c
Merge remote-tracking branch 'origin/NC-1361' into NC-1361
Shinabyss Dec 10, 2018
ae5e2ce
Use default from websocket class instead of making new one
Shinabyss Dec 10, 2018
7b90fea
Merge branch 'master' into NC-1361
Shinabyss Dec 10, 2018
931af28
Merge branch 'master' into NC-1361
Shinabyss Dec 10, 2018
113ba99
Removed magic numbers
Shinabyss Dec 10, 2018
f8dcba8
Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …
Shinabyss Dec 10, 2018
35d3e7d
Merge remote-tracking branch 'origin/NC-1361' into NC-1361
Shinabyss Dec 10, 2018
00497f2
Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …
Shinabyss Dec 11, 2018
d67806d
Made error message use const as well
Shinabyss Dec 11, 2018
79c09e4
Merge branch 'master' into NC-1361
Shinabyss Dec 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,30 @@ public PantheonNode createMinerNode(final String name) throws IOException {
name, createMiningParameters(true), createJsonRpcConfig(), createWebSocketConfig()));
}

public PantheonNode createMinerNodeWithCustomRefreshDelay(
final String name, final Long refreshDelay) throws IOException {
WebSocketConfiguration wsConfig = createWebSocketConfig();
wsConfig.setRefreshDelay(refreshDelay);
return create(
new PantheonFactoryConfiguration(
name, createMiningParameters(true), createJsonRpcConfig(), wsConfig));
}

public PantheonNode createArchiveNode(final String name) throws IOException {
return create(
new PantheonFactoryConfiguration(
name, createMiningParameters(false), createJsonRpcConfig(), createWebSocketConfig()));
}

public PantheonNode createArchiveNodeWithCustomRefreshDelay(
final String name, final Long refreshDelay) throws IOException {
WebSocketConfiguration wsConfig = createWebSocketConfig();
wsConfig.setRefreshDelay(refreshDelay);
return create(
new PantheonFactoryConfiguration(
name, createMiningParameters(false), createJsonRpcConfig(), wsConfig));
}

public PantheonNode createArchiveNodeWithRpcDisabled(final String name) throws IOException {
return create(
new PantheonFactoryConfiguration(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,21 @@ public class WebSocketConfiguration {
public static final int DEFAULT_WEBSOCKET_PORT = 8546;
public static final Collection<RpcApi> DEFAULT_WEBSOCKET_APIS =
Arrays.asList(RpcApis.ETH, RpcApis.NET, RpcApis.WEB3);
public static final long DEFAULT_WEBSOCKET_REFRESH_DELAY = 5000;

private boolean enabled;
private int port;
private String host;
private Collection<RpcApi> rpcApis;
private long refreshDelay;

public static WebSocketConfiguration createDefault() {
final WebSocketConfiguration config = new WebSocketConfiguration();
config.setEnabled(false);
config.setHost(DEFAULT_WEBSOCKET_HOST);
config.setPort(DEFAULT_WEBSOCKET_PORT);
config.setRpcApis(DEFAULT_WEBSOCKET_APIS);
config.setRefreshDelay(DEFAULT_WEBSOCKET_REFRESH_DELAY);
return config;
}

Expand Down Expand Up @@ -110,4 +113,12 @@ public boolean equals(final Object o) {
public int hashCode() {
return Objects.hashCode(enabled, port, host, rpcApis);
}

public void setRefreshDelay(final long refreshDelay) {
this.refreshDelay = refreshDelay;
}

public long getRefreshDelay() {
return refreshDelay;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ public class SubscriptionManager extends AbstractVerticle {
private final Map<Long, Subscription> subscriptions = new HashMap<>();
private final Map<String, List<Long>> connectionSubscriptionsMap = new HashMap<>();
private final SubscriptionBuilder subscriptionBuilder = new SubscriptionBuilder();
private final long refreshDelay;

public SubscriptionManager(final long refreshDelay) {
this.refreshDelay = refreshDelay;
}

public SubscriptionManager() {
this.refreshDelay = 5000;
Copy link
Contributor

Choose a reason for hiding this comment

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

use the const here rather than the number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

@Override
public void start() {
Expand Down Expand Up @@ -162,4 +171,8 @@ public void sendMessage(final Long subscriptionId, final JsonRpcResult msg) {
.findFirst()
.ifPresent(connectionId -> vertx.eventBus().send(connectionId, Json.encode(response)));
}

public long getRefreshDelay() {
return refreshDelay;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ public class SyncingSubscriptionService {

private final SubscriptionManager subscriptionManager;
private final Synchronizer synchronizer;
private final long currentRefreshDelay = 5000;

private Optional<SyncStatus> previousSyncStatus;
private long timerId;
Expand Down Expand Up @@ -76,7 +75,7 @@ public void engageNextTimerTick() {
subscriptionManager
.getVertx()
.setTimer(
currentRefreshDelay,
subscriptionManager.getRefreshDelay(),
(id) -> {
sendSyncingToMatchingSubscriptions();
engageNextTimerTick();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ public Runner build() {
filterManager);

final SubscriptionManager subscriptionManager =
createSubscriptionManager(vertx, transactionPool);
createSubscriptionManager(
vertx, transactionPool, webSocketConfiguration.getRefreshDelay());

createLogsSubscriptionService(
context.getBlockchain(), context.getWorldStateArchive(), subscriptionManager);
Expand Down Expand Up @@ -324,8 +325,8 @@ private Map<String, JsonRpcMethod> jsonRpcMethods(
}

private SubscriptionManager createSubscriptionManager(
final Vertx vertx, final TransactionPool transactionPool) {
final SubscriptionManager subscriptionManager = new SubscriptionManager();
final Vertx vertx, final TransactionPool transactionPool, final long refreshDelay) {
final SubscriptionManager subscriptionManager = new SubscriptionManager(refreshDelay);
final PendingTransactionSubscriptionService pendingTransactions =
new PendingTransactionSubscriptionService(subscriptionManager);
transactionPool.addTransactionListener(pendingTransactions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

if (refreshDelay < 1 || refreshDelay > 3600000) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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)");
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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>",
Expand Down Expand Up @@ -504,6 +524,7 @@ private WebSocketConfiguration webSocketConfiguration() {
webSocketConfiguration.setHost(wsHostAndPort.getHost());
webSocketConfiguration.setPort(wsHostAndPort.getPort());
webSocketConfiguration.setRpcApis(wsApis);
webSocketConfiguration.setRefreshDelay(refreshDelay);
return webSocketConfiguration;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,28 @@ public void bootnodesOptionMustBeUsed() {
assertThat(commandErrorOutput.toString()).isEmpty();
}

@Test
public void callingWithRefreshDelayWithValueMustUseValue() {
parseCommand("--ws-refresh-delay", "2000");

verify(mockRunnerBuilder).webSocketConfiguration(wsRpcConfigArgumentCaptor.capture());
verify(mockRunnerBuilder).build();

assertThat(wsRpcConfigArgumentCaptor.getValue().getRefreshDelay()).isEqualTo(2000);

assertThat(commandOutput.toString()).isEmpty();
assertThat(commandErrorOutput.toString()).isEmpty();
}

@Test
public void callingWithRefreshDelayWithNegativeValueMustError() {
parseCommand("--ws-refresh-delay", "-2000");

assertThat(commandOutput.toString()).isEmpty();
final String expectedErrorMsg = "refreshDelay must be a positive integer";
assertThat(commandErrorOutput.toString()).startsWith(expectedErrorMsg);
}

@Test
public void callingWithNodesWhitelistOptionButNoValueMustNotError() {
parseCommand("--nodes-whitelist");
Expand Down