Retry ChunkInsert and tune retry settings #51
Conversation
bbuchalter
requested review from
jordanwheeler,
insom,
berlincount and
sroysen
|
This PR depends on a feature introduced in |
This is a re-implementation of the RetryHelper that was introduced in #39. By being a class instead of a module, it has a much more clear interface and requires less setup. It features a true integration test for lock conditions which were replaced in #20 with mocks. It's configuration is currently identical to the configuration of RetryHelper.
As we have all components use SqlRetry, we're going to need better logging to know what is being retried. Copying the whole proc for the retry log message into each component's configuration is going to be very painful. Let's add better support for this into SqlRetry.
The retry helper assumes access to a @connection, so it is better to pass this in when initializing than in a particular method.
After a lengthy conversation an exploration in #44 it's been decided it's best to err on the side of being conservative and retry only 10 times. This can be adjusted and tuned later.
These are also common causes of the need to manually retry. Let's automate the retry of these error messages too.
We rely on kamui/retriable#24 with our default configuration of retriable which was released in 3.0.0.
bbuchalter
force-pushed
the
retry_refactor
branch
from
b8d8fc4 to
34fcd55
Compare
There was a problem hiding this comment.
This is great 👍
I appreciate the comments around the testing of LWTOs in timeouts too -- it's complicated and the comments are short but have the important information in them, so: cool.
base_interval went from having "opinionated defaults" to just being 1s everywhere unless overridden -- do you see us moving that all to Shopify/shopify and Shopify/shopify_lhm in the end? I'm not sure that the default of 1s is great, but I don't want to bike shed here if you expect we'll set this elsewhere.
Also I don't love the
{}.merge!(options.fetch(:something, {}))pattern, but at the same time -- I don't have a better syntax suggestion so ... 🚢? 😁
The problem with this right now is that it can't be overridden by end users of the LHM library. There needs to be external configuration exposed for this at the library level. Right now the only configuration exposed is internally, to make tests not slow. This is a separate issue that I've filed #53. |
NOTE: This PR is best reviewed commit by commit.
This PR has three parts:
SqlRetryas a replacement forRetryHelper.ChunkerInsert,AtomicSwitcher,Entanglerto all useSqlRetry.