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

Update IbftConfig Fields #422

Merged
merged 8 commits into from
Dec 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -20,7 +20,7 @@ public class IbftConfigOptions {

private static final long DEFAULT_EPOCH_LENGTH = 30_000;
private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 1;
private static final int DEFAULT_ROUND_EXPIRY_MILLISECONDS = 10000;
private static final int DEFAULT_ROUND_EXPIRY_SECONDS = 1;

private final JsonObject ibftConfigRoot;

Expand All @@ -36,7 +36,7 @@ public int getBlockPeriodSeconds() {
return ibftConfigRoot.getInteger("blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
}

public int getRequestTimeoutMillis() {
return ibftConfigRoot.getInteger("requesttimeout", DEFAULT_ROUND_EXPIRY_MILLISECONDS);
public int getRequestTimeoutSeconds() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name requestTimeout doesn't really indicate what it is for. Some mention of the round in name would be good. Perhaps something like initialRoundTimeoutSeconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but can we worry about this one next, don't want to stray TOO far right now.

return ibftConfigRoot.getInteger("requesttimeoutseconds", DEFAULT_ROUND_EXPIRY_SECONDS);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class IbftConfigOptionsTest {

private static final int EXPECTED_DEFAULT_EPOCH_LENGTH = 30_000;
private static final int EXPECTED_DEFAULT_BLOCK_PERIOD = 1;
private static final int EXPECTED_DEFAULT_REQUEST_TIMEOUT = 10_000;
private static final int EXPECTED_DEFAULT_REQUEST_TIMEOUT = 1;

@Test
public void shouldGetEpochLengthFromConfig() {
Expand Down Expand Up @@ -64,19 +64,19 @@ public void shouldGetDefaultBlockPeriodFromDefaultConfig() {

@Test
public void shouldGetRequestTimeoutFromConfig() {
final IbftConfigOptions config = fromConfigOptions(singletonMap("RequestTimeout", 5));
assertThat(config.getRequestTimeoutMillis()).isEqualTo(5);
final IbftConfigOptions config = fromConfigOptions(singletonMap("RequestTimeoutSeconds", 5));
assertThat(config.getRequestTimeoutSeconds()).isEqualTo(5);
}

@Test
public void shouldFallbackToDefaultRequestTimeout() {
final IbftConfigOptions config = fromConfigOptions(emptyMap());
assertThat(config.getRequestTimeoutMillis()).isEqualTo(EXPECTED_DEFAULT_REQUEST_TIMEOUT);
assertThat(config.getRequestTimeoutSeconds()).isEqualTo(EXPECTED_DEFAULT_REQUEST_TIMEOUT);
}

@Test
public void shouldGetDefaultRequestTimeoutFromDefaultConfig() {
assertThat(IbftConfigOptions.DEFAULT.getRequestTimeoutMillis())
assertThat(IbftConfigOptions.DEFAULT.getRequestTimeoutSeconds())
.isEqualTo(EXPECTED_DEFAULT_REQUEST_TIMEOUT);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

/** Execution context for draining queued ibft events and applying them to a maintained state */
public class IbftProcessor implements Runnable {

private static final Logger LOG = LogManager.getLogger();

private final IbftEventQueue incomingQueue;
Expand All @@ -35,32 +36,33 @@ public class IbftProcessor implements Runnable {
* Construct a new IbftProcessor
*
* @param incomingQueue The event queue from which to drain new events
* @param baseRoundExpiryMillis The expiry time in milliseconds of round 0
* @param baseRoundExpirySeconds The expiry time in milliseconds of round 0
* @param stateMachine an IbftStateMachine ready to process events and maintain state
*/
public IbftProcessor(
final IbftEventQueue incomingQueue,
final int baseRoundExpiryMillis,
final int baseRoundExpirySeconds,
final IbftStateMachine stateMachine) {
// Spawning the round timer with a single thread as we should never have more than 1 timer in
// flight at a time
this(
incomingQueue,
baseRoundExpiryMillis,
baseRoundExpirySeconds,
stateMachine,
Executors.newSingleThreadScheduledExecutor());
}

@VisibleForTesting
IbftProcessor(
final IbftEventQueue incomingQueue,
final int baseRoundExpiryMillis,
final int baseRoundExpirySeconds,
final IbftStateMachine stateMachine,
final ScheduledExecutorService roundTimerExecutor) {
this.incomingQueue = incomingQueue;
this.roundTimerExecutor = roundTimerExecutor;

this.roundTimer = new RoundTimer(incomingQueue, baseRoundExpiryMillis, roundTimerExecutor);
this.roundTimer =
new RoundTimer(incomingQueue, baseRoundExpirySeconds * 1000, roundTimerExecutor);
this.stateMachine = stateMachine;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,17 @@ public class RoundTimer {
* Construct a RoundTimer with primed executor service ready to start timers
*
* @param queue The queue in which to put round expiry events
* @param baseExpiryMillis The initial round length for round 0
* @param baseExpirySeconds The initial round length for round 0
* @param timerExecutor executor service that timers can be scheduled with
*/
public RoundTimer(
final IbftEventQueue queue,
final long baseExpiryMillis,
final long baseExpirySeconds,
final ScheduledExecutorService timerExecutor) {
this.queue = queue;
this.timerExecutor = timerExecutor;
this.currentTimerTask = Optional.empty();
this.baseExpiryMillis = baseExpiryMillis;
this.baseExpiryMillis = baseExpirySeconds * 1000;
}

/** Cancels the current running round timer if there is one */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,22 @@ public void cancelTimerCancelsWhenNoTimer() {

@Test
public void startTimerSchedulesTimerCorrectlyForRound0() {
checkTimerForRound(0, 1);
checkTimerForRound(0, 1000);
}

@Test
public void startTimerSchedulesTimerCorrectlyForRound1() {
checkTimerForRound(1, 2);
checkTimerForRound(1, 2000);
}

@Test
public void startTimerSchedulesTimerCorrectlyForRound2() {
checkTimerForRound(2, 4);
checkTimerForRound(2, 4000);
}

@Test
public void startTimerSchedulesTimerCorrectlyForRound3() {
checkTimerForRound(3, 8);
checkTimerForRound(3, 8000);
}

private void checkTimerForRound(final int roundNumber, final long timeout) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public static PantheonController<IbftContext> init(

final IbftStateMachine ibftStateMachine = new IbftStateMachine(blockCreatorFactory);
final IbftProcessor ibftProcessor =
new IbftProcessor(ibftEventQueue, ibftConfig.getRequestTimeoutMillis(), ibftStateMachine);
new IbftProcessor(ibftEventQueue, ibftConfig.getRequestTimeoutSeconds(), ibftStateMachine);
Copy link
Contributor

Choose a reason for hiding this comment

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

Far side of this call is still referring to it's parameter as millis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

final ExecutorService processorExecutor = Executors.newSingleThreadExecutor();
processorExecutor.submit(ibftProcessor);

Expand Down
2 changes: 1 addition & 1 deletion pantheon/src/test/resources/ibftlegacy_genesis.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"ibft": {
"epochLength": 30000,
"blockPeriodSeconds" : 1,
"requestTimeout": 10000
"requestTimeoutSeconds": 10
}
},
"nonce": "0x0",
Expand Down