-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
offchain - remove dependency on pg.qopts #727
Conversation
…into dk/rm-dep-pgqopts
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.
Looks incomplete. Were you planning to leave these TODO's?
@@ -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) |
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.
missed
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.
@@ -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) |
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.
missed
@@ -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) |
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.
missed
@@ -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) |
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.
missed
@@ -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) |
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.
missed
@@ -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) |
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.
missed
@@ -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) |
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.
missed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Is there any type of test we should write or manually test something to make sure that we don't introduce any regression here? Any kind of e2e test to verify that context is indeed propagated top-down?
Check
changeset
.