-
Notifications
You must be signed in to change notification settings - Fork 130
[MINOR / PAN-1339 (A)] Fetch local transactions in isolation #1259
Conversation
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.
LGTM. 2 possible improvements comments.
@@ -88,6 +88,18 @@ public PendingTransactions( | |||
"operation"); | |||
} | |||
|
|||
List<Transaction> getLocalTransactions() { |
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.
Maybe we could do it using parallel stream to optimize.
List<Transaction> getLocalTransactions() { | |
List<Transaction> getLocalTransactions() { | |
synchronized (pendingTransactions) { | |
List<Transaction> localTransactions = new ArrayList<>(); | |
pendingTransactions | |
.entrySet() | |
.parallelStream() | |
.filter(transaction -> transaction.getValue().isReceivedFromLocalSource()) | |
.forEach(transaction -> transaction.getValue().getTransaction()); | |
return localTransactions; | |
} | |
} |
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.
Suggestion tip : cache the filtered list and store a boolean indicating whether the list is still consistent or not. Set it to false when a new transaction is received. We could have a method invalidateCachedLocalTransactions()
to set this boolean to false when appropriate.
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.
Class level field to avoid instantiating localTransactions
each time localTransactions
is called ?
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.
I'd avoid any such caching until we know we have a performance issue. We're currently having issues with memory usage so don't want to waste it on extra caches if they aren't necessary.
PR description
In an effort to make the change set inevitably necessitated by the acceptance criteria of
PAN-1339
less onerous, and consequently the review more palatable, I've decided to serialize it into constituent elements -- each of which constituting a relatively discrete semantic chunk.Fixed Issue(s)
In order to disseminate local transactions to new peers we'll require a mechanism to fetch local transactions in isolation -- this is that mechanism (along with associated test infrastructure).