fix: (log_poller backfill) exit on non-rpc err #14114
Conversation
bukata-sa
requested review from
reductionista,
andrevmatos,
jmank88 and
a team
and removed request for
a team and
jmank88
bukata-sa
force-pushed
the
fix/backfill_err_exit
branch
from
14597fb
to
389875c
Compare
| var rpcErr *client.JsonError | ||
| if !pkgerrors.As(err, &rpcErr) || rpcErr.Code != jsonRpcLimitExceeded { | ||
| lp.lggr.Errorw("Unable to query for logs", "err", err, "from", from, "to", to) | ||
| return err |
There was a problem hiding this comment.
I initially tried to make this change in Dec 2023, but during review it became clear that this is a somewhat risky change and there was more to investigate about what the right way is to fix it. I re-opened it a few weeks ago, and got a more complex fix working that actually detects the error correctly (at least on some rpc servers.) This by itself will not, because it's comparing against a concrete type that will only match during testing, not in production.
I thought that was ready to merge last week, but unfortunately during review it again became clear that things are more complicated. There are so many different error codes and string formats returned from different rpc servers for the same condition we'll have to rely mostly on string parsing to differentiate them.
Leaving it as-is at least errs on the side of caution because, even though the logic is flawed, it's just taking preventative measures when it doesn't actually need to in most cases... and reporting a critical error in some cases when it shouldn't. Not taking those preventative measures, or not reporting a critical condition is more risky in production.
Is there a way to fix the flaky test without making this change?
Here is the current PR as it stands:
#11654
|
What
Properly detect error type and exit on non-rpc error
Why
On graceful shutdown backfill method reacts on
context cancelederror as rpc batch size error, which leads tocritlog and failed testExample log cl-node-54af8721.log
Resolves Dependencies
#13647