offchain - remove dependency on pg.qopts #727
Conversation
…into dk/rm-dep-pgqopts
| @@ -244,7 +243,7 @@ func (c *CommitStore) ChangeConfig(_ context.Context, onchainConfig []byte, offc | |||
| } | |||
|
|
|||
| func (c *CommitStore) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(c.lp, c.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), c.lp, c.filters) | |||
| @@ -468,7 +467,7 @@ func (o *OffRamp) ChangeConfig(ctx context.Context, onchainConfigBytes []byte, o | |||
| } | |||
|
|
|||
| func (o *OffRamp) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(o.lp, o.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), o.lp, o.filters) | |||
| @@ -194,11 +194,11 @@ func (o *OnRamp) GetUSDCMessagePriorToLogIndexInTx(ctx context.Context, logIndex | |||
| } | |||
|
|
|||
| func (o *OnRamp) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(o.lp, o.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), o.lp, o.filters) | |||
| @@ -147,7 +147,7 @@ func (p *PriceRegistry) GetFeeTokens(ctx context.Context) ([]cciptypes.Address, | |||
| } | |||
|
|
|||
| func (p *PriceRegistry) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(p.lp, p.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), p.lp, p.filters) | |||
| @@ -250,7 +250,7 @@ func (c *CommitStore) ChangeConfig(_ context.Context, onchainConfig []byte, offc | |||
| } | |||
|
|
|||
| func (c *CommitStore) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(c.lp, c.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), c.lp, c.filters) | |||
| @@ -208,11 +208,11 @@ func (o *OnRamp) IsSourceCursed(ctx context.Context) (bool, error) { | |||
| } | |||
|
|
|||
| func (o *OnRamp) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(o.lp, o.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), o.lp, o.filters) | |||
| @@ -206,11 +206,11 @@ func (o *OnRamp) IsSourceCursed(ctx context.Context) (bool, error) { | |||
| } | |||
|
|
|||
| func (o *OnRamp) Close() error { | |||
| return logpollerutil.UnregisterLpFilters(o.lp, o.filters) | |||
| return logpollerutil.UnregisterLpFilters(context.TODO(), o.lp, o.filters) | |||
| func (u *USDCReaderImpl) Close(qopts ...pg.QOpt) error { | ||
| // FIXME Dim pgOpts removed from LogPoller | ||
| return u.lp.UnregisterFilter(context.Background(), u.filter.Name) | ||
| func (u *USDCReaderImpl) Close(ctx context.Context) error { |
There was a problem hiding this comment.
@krehermann Were you fiddling with whether Close should accept context recently?
It's an odd case because we have already cancelled most relevant contexts, and are actually triggering these during cleanup, so we don't want to pass cancelled contexts. On the other hand, some of these calls end up going over GRPC, rather than just closing the local client, so a new context has to be created somewhere. Do we have a standard short timeout to apply in that case? So we don't block shutdown?
There was a problem hiding this comment.
i was considering Close(ctx) because of the existing pattern to UnregisterFilters from Logpoller as part of the Close. That LP call used to take a pg.Opt and the Close had been ~ Close(pg.Opts...). This enabled transactional semantics in the DB. The use case was CLO job creation and deletion - for some unclear reason (to me) creating and deleting CCIP jobs was transactionally coupled to logpoller filters.
My main concern with this change is to avoid introducing a regression there.
As far as Close is concerned: Close(ctx) will not work when the EVM relayer is a LOOPP. As you have said offline @jmank88 : even if we were to naively port Close to be Close(ctx), we will not maintain transactional semantics across process boundaries
Given all of this I do not think it's a good idea to make Close(ctx), because we will have to undo it shortly as part of the EVM migration
@dimkouv does the change here still work with CLO? Are there tests that prove it? this is related to @mateusz-sekara comment about e2e tests.
@roman-kashitsyn made a change to decouple the log poller registration from job creation, but it was rolled back 4194fac. Given that his work was committed and reverted, I suspect the existing tests are incomplete. @roman-kashitsyn can you comment on your experience with lazy initialization of the log poller dependencies?
Check
changeset.