Prune old log poller blocks - #7619
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
…inlink into lp-prune-old-blocks
makramkd
left a comment
makramkd
left a comment
There was a problem hiding this comment.
LGTM - two minor comments
| // How fast that can be done depends largely on network speed and DB, but even for the fastest | ||
| // support chain, polygon, which has 2s block times, we need RPCs roughly with <= 500ms latency | ||
| func NewLogPoller(orm *ORM, ec client.Client, lggr logger.Logger, pollPeriod time.Duration, finalityDepth, backfillBatchSize, rpcBatchSize int64) *logPoller { | ||
| func NewLogPoller(orm *ORM, ec client.Client, lggr logger.Logger, pollPeriod time.Duration, finalityDepth int64, backfillBatchSize int64, rpcBatchSize int64, keepBlocksDepth int64) *logPoller { |
There was a problem hiding this comment.
Minor note/nit #1: This constructor signature is quite long, worth doing something to shorten it? We don't really have a paradigm for this in core, but just something I noticed in other places as well. In other large Go codebases I've worked on, we actually didn't have these New* methods at all and almost all fields were public (but this was also the case because we were using https://github.com/uber-go/fx).
Minor note/nit #2: this Newer returns the private *logPoller, but wouldn't it be more idiomatic if it returned the public interface type instead? (LogPoller)
There was a problem hiding this comment.
1 - I like explicit args as its just harder to misuse, you can't forget to specify a struct field this way. Lots of internal fields that I wouldn't want to expose (cached values etc.). Std go library has NewXX constructors all over it.
2 - Its more flexible to return the struct, callers can implicitly cast as needed e.g. var lp LogPoller = NewLogPoller. General go idiom is "accept interfaces, return structs"
| logPollTick := time.After(0) | ||
| blockPruneTick := time.After(0) |
There was a problem hiding this comment.
Just curiosity, but can you explain this? AFAIK go's select nondeterministically selects a branch (see https://go.dev/ref/spec#Select_statements):
If one or more of the communications can proceed, a single one that can proceed is chosen via a uniform pseudo-random selection.
What's the benefit of doing this vs. setting them to the appropriate durations straight away here?
There was a problem hiding this comment.
it is indeed random - no real benefit imo. I didn't see much of a difference between pruning or begin polling on lp boot
2 participants
On very fast chains, if the node stays up and has the log poller running, the performance will degrade as log_poller_blocks builds up. For example a 1s block time would produce 86400 block entries per day. This adds a periodic pruning to remove blocks more than keepBlocksDepth behind the head.