[PAN-2327] Notify of dropped messages #1156
Conversation
rojotek
changed the title
rojotek
force-pushed
the
notify-of-dropped-messages
branch
from
f716b5e
to
2b360e1
Compare
| @@ -54,25 +58,61 @@ | |||
| private final Collection<PendingTransactionListener> listeners = | |||
| newSetFromMap(new ConcurrentHashMap<>()); | |||
|
|
|||
| private final Collection<PendingTransactionDroppedListener> transactionDroppedListeners = | |||
| newSetFromMap(new ConcurrentHashMap<>()); | |||
There was a problem hiding this comment.
Could this please use the Subscribers class? It's specifically designed to be the pattern for tracking listeners.
| "remoteTransactionCounter", | ||
| "count of remote created transactions in the pending transaction pool", | ||
| "taskName") | ||
| .labels(getClass().getSimpleName()); |
There was a problem hiding this comment.
These metrics aren't quite right. I'd suggest:
final LabelledMetric<Counter> transactionCounter = metricsSystem
.createLabelledCounter(
MetricCategory.TRANSACTION_POOL,
"transactions_total",
"Total number of transactions added to the pending transaction pool",
"source");
this.localTransactionCounter = transactionCounter.label("local");
this.remoteTransactionCounter = transactionCounter.label("remote");
We don't need or want the class name in the metrics at all and the advantage of using a single labelled metric for both local and remote is it indicates that these are very related (its easy to add them together to see all transactions).
| if (removedTransactionInfo.isReceivedFromLocalSource()) { | ||
| localTransactionCounter.inc(-1); | ||
| } else { | ||
| remoteTransactionCounter.inc(-1); |
There was a problem hiding this comment.
Counters should never be decremented, they only go up. We should have a separate "transactions_dropped_total" counter that we increment here (probably with a "source" label for local & remote).
|
|
||
| for (final Subscription subscription : subscriptions) { | ||
| subscriptionManager.sendMessage( | ||
| subscription.getId(), new PendingTransactionResult(pendingTransaction)); |
There was a problem hiding this comment.
Should we create the PendingTransactionResult once and reuse it to send to each subscription?
rojotek
force-pushed
the
notify-of-dropped-messages
branch
from
0c6cb12
to
77d2a56
Compare
| final String addedToBlock) { | ||
| transactionRemovedCounters.put( | ||
| new DecrementTransactionKey("local".equals(source), "addedToBlock".equals(addedToBlock)), | ||
| transactionCounter.labels(source, "removed", addedToBlock)); |
There was a problem hiding this comment.
I think you've probably over-optimised this (probably because of my previous suggestions, sorry). It's ok to call .labels multiple times. It's very slightly faster if you can just store the result like with localTransactionAddedCounter and removeTransactionAddedCounter. Given there's both a local/remote and addedToBlock/dropped axis, I'd just store the LabelledMetric<Counter> and then when you need to increment call .labels(...).inc(). The indirection of looking up in a map likely nullifies any benefit to the caching.
rojotek
force-pushed
the
notify-of-dropped-messages
branch
2 times, most recently
from
647db57
to
247fc44
Compare
…on pending pool. Implemented subscription webservice to support this.
…al and remote transactions in the pool.
…ment the appropriate one when transactions are removed from the pool.
rojotek
force-pushed
the
notify-of-dropped-messages
branch
from
247fc44
to
0397618
Compare
Added ability to subscribe to dropped transactions from the transaction pending pool.
Implemented subscription webservice to support this.
Added metrics to the pending transactions, tracking the number of local and remote transactions in the pool.