[PAN-1062] Specify pending transaction retention period (2 of 2) #1333

smatthewenglish · 2019-04-25T15:44:58Z

PR description

The acceptance criteria for this task has two components:

Configuration option to specify maximum length of time transactions are held in the pending transaction list before being dropped
When a transaction has been in the pending transaction list for the configured maximum period, it is removed

This change is intended to address the former.

Fixed Issue(s)

Specify at the command line (PantheonCommand) the maximum length of time transactions are held in the pending transaction list before being dropped.

shemnon · 2019-04-25T21:14:00Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java

@@ -80,6 +80,7 @@ public void startNode(final PantheonNode node) {
                  new RocksDbConfiguration.Builder().databaseDir(tempDir).build())
              .ethereumWireProtocolConfiguration(EthereumWireProtocolConfiguration.defaultConfig())
              .clock(Clock.systemUTC())
+              .pendingTransactionRetentionPeriod(PendingTransactions.PENDING_TX_RETENTION_PERIOD)


I would move this up to between 78 and 79 since they touch the same part of config. In genreral wherever we do this call as well.

shemnon · 2019-04-25T21:15:10Z

...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

@@ -48,6 +48,7 @@
 * <p>This class is safe for use across multiple threads.
 */
 public class PendingTransactions {
+  public static final int PENDING_TX_RETENTION_PERIOD = 13;


Need a unit in the name. Seconds? Minutes? Hours? PENDING_TX_RETENTION_HOURS?

shemnon · 2019-04-25T21:15:26Z

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java

@@ -154,7 +154,9 @@ public TestNode(
            TestClock.fixed(),
            PendingTransactions.MAX_PENDING_TRANSACTIONS,
            metricsSystem,
-            syncState);
+            syncState,
+            13);


use the constant.

shemnon · 2019-04-25T21:15:37Z

...eum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/EthProtocolManagerTest.java

@@ -1056,7 +1056,8 @@ public void transactionMessagesGoToTheCorrectExecutor() {
          TestClock.fixed(),
          PendingTransactions.MAX_PENDING_TRANSACTIONS,
          metricsSystem,
-          mock(SyncState.class));
+          mock(SyncState.class),
+          13);


Use the constant.

shemnon · 2019-04-25T21:16:08Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/DefaultCommandValues.java

@@ -34,6 +34,7 @@
  String PANTHEON_HOME_PROPERTY_NAME = "pantheon.home";
  String DEFAULT_DATA_DIR_PATH = "./build/data";
  String MANDATORY_INTEGER_FORMAT_HELP = "<INTEGER>";
+  String PENDING_TX_RETENTION_PERIOD_FORMAT_HELP = "<INTEGER>h<INTEGER>m<INTEGER>s";


This would be just DURATION_HELP if we parse into the duration object.

shemnon · 2019-04-25T21:16:29Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

+      names = {"--tx-pool-retention-period"},
+      paramLabel = PENDING_TX_RETENTION_PERIOD_FORMAT_HELP,
+      description =
+          "Maximum retention period of pending transactions in the transaction pool (default: ${DEFAULT-VALUE})",


Need unit in the text if we stay at long/integer.

shemnon · 2019-04-25T21:16:51Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -705,6 +713,7 @@ private void ensureAllNodesAreInWhitelist(
          .metricsSystem(metricsSystem.get())
          .privacyParameters(privacyParameters())
          .clock(Clock.systemUTC())
+          .pendingTransactionRetentionPeriod(pendingTxRetentionPeriod)


Again, move next to maxPendingTransactions.

shemnon · 2019-04-25T21:17:19Z

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

@@ -77,6 +77,10 @@
  private final List<Runnable> shutdownActions = new ArrayList<>();
  private RocksDbConfiguration rocksdDbConfiguration;

+  /* * */
+  protected Integer pendingTransactionRetentionPeriod;


Again, adjacent to maxPendingTransactions. Also, needs a unit or needs to go to Duration. One of those.

shemnon · 2019-04-25T21:17:31Z

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

@@ -151,6 +155,12 @@
    return this;
  }

+  public PantheonControllerBuilder<C> pendingTransactionRetentionPeriod(


Again, adjacency.

my bad but -- what do you mean by adjacency? maybe you mean move it near to maxPendingTransactions, but already it is

prior to the force push it wasn't

ah, yeah- I always have to force push after I rebase - is there a better way to do that?

I tend to use git pull upstream master --no-rebase when pulling changes from master after I've created the PR the first time. Avoids the need to force push.

shemnon · 2019-04-25T21:17:57Z

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

@@ -215,6 +225,10 @@
      PeerValidatorRunner.runValidator(ethContext, daoForkPeerValidator);
    }

+    if (pendingTransactionRetentionPeriod == null) {


Looks like stray debug code.

shemnon · 2019-04-29T16:51:53Z

...th/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolFactory.java

@@ -33,7 +33,12 @@ public static TransactionPool createTransactionPool(
      final Clock clock,
      final int maxPendingTransactions,
      final MetricsSystem metricsSystem,
-      final SyncState syncState) {
+      final SyncState syncState,
+      final int pendingTransactionRetentionPeriod) {


This argument needs to be grouped next to maxPendingTransactions. between line 34 and 35

shemnon · 2019-04-29T16:53:01Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/DefaultCommandValues.java

@@ -34,6 +34,7 @@
  String PANTHEON_HOME_PROPERTY_NAME = "pantheon.home";
  String DEFAULT_DATA_DIR_PATH = "./build/data";
  String MANDATORY_INTEGER_FORMAT_HELP = "<INTEGER>";
+  String DURATION_HELP = "<INTEGER>";


Unless it's a new string simply re-use the existing generic strings. This can be deleted until we go to duration objects.

shemnon · 2019-04-29T16:53:29Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -483,6 +483,14 @@ void setBootnodes(final List<String> values) {
      arity = "1")
  private final Integer txPoolMaxSize = PendingTransactions.MAX_PENDING_TRANSACTIONS;

+  @Option(
+      names = {"--tx-pool-retention-hours"},
+      paramLabel = DURATION_HELP,


Suggested change

paramLabel = DURATION_HELP,

paramLabel = MANDATORY_INTEGER_FORMAT_HELP,

shemnon · 2019-04-29T16:55:58Z

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

@@ -151,6 +155,12 @@
    return this;
  }

+  public PantheonControllerBuilder<C> pendingTransactionRetentionPeriod(


prior to the force push it wasn't

shemnon · 2019-04-29T16:56:22Z

pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java

@@ -133,6 +133,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
            .clock(TestClock.fixed())
            .maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
            .storageProvider(createKeyValueStorageProvider(dbAhead))
+            .pendingTransactionRetentionPeriod(PendingTransactions.PENDING_TX_RETENTION_HOURS)


Move up a line, next to maxPendingTransacitons.

shemnon · 2019-04-29T16:56:36Z

pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java

@@ -152,6 +153,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
            .clock(TestClock.fixed())
            .maxPendingTransactions(PendingTransactions.MAX_PENDING_TRANSACTIONS)
            .storageProvider(createKeyValueStorageProvider(dbAhead))
+            .pendingTransactionRetentionPeriod(PendingTransactions.PENDING_TX_RETENTION_HOURS)


Move up a line, next to maxPendingTransacitons.

ajsutton

LGTM. Just a couple of nits to sort out.

ajsutton · 2019-04-30T03:16:33Z

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

@@ -151,6 +155,12 @@
    return this;
  }

+  public PantheonControllerBuilder<C> pendingTransactionRetentionPeriod(


I tend to use git pull upstream master --no-rebase when pulling changes from master after I've created the PR the first time. Avoids the need to force push.

ajsutton · 2019-04-30T03:19:20Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

+      names = {"--tx-pool-retention-hours"},
+      paramLabel = MANDATORY_INTEGER_FORMAT_HELP,
+      description =
+          "Maximum retention period of pending transactions, expressed in hours. Represented by primitive type 'long'. (default: ${DEFAULT-VALUE})",


nit: drop the redundant word 'expressed' and user's don't care how we represent the value internally.

Suggested change

"Maximum retention period of pending transactions, expressed in hours. Represented by primitive type 'long'. (default: ${DEFAULT-VALUE})",

"Maximum retention period of pending transactions in hours (default: ${DEFAULT-VALUE})",

ajsutton · 2019-04-30T03:20:51Z

pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java

+
+    verify(mockControllerBuilder).pendingTransactionRetentionPeriod(intArgumentCaptor.capture());
+
+    assertThat(intArgumentCaptor.getAllValues().get(0)).isEqualTo(pendingTxRetentionHours);


You don't need to use the int argument captor here - the verify(mockControllerBuilder).pendingTransactionRetentionPeriod(eq(pendingTxRetentionHours)); line below is enough to assert that the method was called with the argument we expect. You'd only use an argument captor when you don't know what the argument will be but want to check it's the same value used in some other place.

…aSysEng#1333)

smatthewenglish changed the title ~~[PAN-1062] Evict old transactions (2 of 2)~~ [PAN-1062] Specify pending transaction retention period (2 of 2) Apr 25, 2019

smatthewenglish force-pushed the tx-period branch from de9376e to 3d7079f Compare April 25, 2019 17:44

smatthewenglish added the work in progress Work on this pull request is ongoing label Apr 25, 2019

shemnon reviewed Apr 25, 2019

View reviewed changes

smatthewenglish force-pushed the tx-period branch 2 times, most recently from 8e6b535 to 1533063 Compare April 29, 2019 15:38

shemnon reviewed Apr 29, 2019

View reviewed changes

smatthewenglish force-pushed the tx-period branch 2 times, most recently from e05f1e1 to 742044a Compare April 29, 2019 20:28

smatthewenglish added 21 commits April 29, 2019 21:39

pendingTransactionRetentionPeriod

ebc437d

PENDING_TX_RETENTION_PERIOD

c5712d0

test

3f7ffc4

CommandTestAbstract

f7a9c9f

13

abbcabd

x

7727ab4

x

45c1faf

toml

3866b1b

x

06bce30

0

e17dcaf

PENDING_TX_RETENTION_HOURS

f42344d

knst

b86c8c6

kn

b94a4f4

x

559fea5

~

feb92a2

*

5f67f24

int

11a9274

pendingTransactionRetentionPeriod

2d9a7d6

ChronoUnit.HOURS

a5967e7

x

7734e0d

DEFAULT_TX_RETENTION_HOURS >< 2

d6b0e53

smatthewenglish added 3 commits April 29, 2019 21:39

debugging

b460f19

clock.step(2L, ChronoUnit.HOURS)

c64c680

update

b888bf4

smatthewenglish force-pushed the tx-period branch from 4daf3fb to b888bf4 Compare April 30, 2019 01:40

update update

97482fb

smatthewenglish removed the work in progress Work on this pull request is ongoing label Apr 30, 2019

ajsutton approved these changes Apr 30, 2019

View reviewed changes

x

1d3eb99

smatthewenglish merged commit a3bff1d into PegaSysEng:master Apr 30, 2019

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019

[PAN-1062] Specify pending transaction retention period (2 of 2) (Peg…

0090901

…aSysEng#1333)

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019

[PAN-1062] Specify pending transaction retention period (2 of 2) (Peg…

9975195

…aSysEng#1333)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-1062] Specify pending transaction retention period (2 of 2) #1333

[PAN-1062] Specify pending transaction retention period (2 of 2) #1333

smatthewenglish commented Apr 25, 2019 •

edited

Loading

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

shemnon Apr 25, 2019

smatthewenglish Apr 26, 2019

shemnon Apr 29, 2019

smatthewenglish Apr 30, 2019

ajsutton Apr 30, 2019

shemnon Apr 25, 2019

shemnon Apr 29, 2019

shemnon Apr 29, 2019

shemnon Apr 29, 2019

shemnon Apr 29, 2019

shemnon Apr 29, 2019

shemnon Apr 29, 2019

ajsutton left a comment

ajsutton Apr 30, 2019

ajsutton Apr 30, 2019

ajsutton Apr 30, 2019

	paramLabel = DURATION_HELP,
	paramLabel = MANDATORY_INTEGER_FORMAT_HELP,

	"Maximum retention period of pending transactions, expressed in hours. Represented by primitive type 'long'. (default: ${DEFAULT-VALUE})",
	"Maximum retention period of pending transactions in hours (default: ${DEFAULT-VALUE})",


		verify(mockControllerBuilder).pendingTransactionRetentionPeriod(intArgumentCaptor.capture());

		assertThat(intArgumentCaptor.getAllValues().get(0)).isEqualTo(pendingTxRetentionHours);

[PAN-1062] Specify pending transaction retention period (2 of 2) #1333

[PAN-1062] Specify pending transaction retention period (2 of 2) #1333

Conversation

smatthewenglish commented Apr 25, 2019 • edited Loading

PR description

Fixed Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented Apr 25, 2019 •

edited

Loading