ingest retry strategy refactor

[rbiseck3]

Description

Pivot from using the retry logic as a decorator as this posed too many limitations on what can be passed in as a parameter at runtime. Moved this to be a class approach and now that can be instantiated with appropriate loggers leveraging the --verbose flag to set the log level. This also mitigates how much new code is being forked from the backoff library. The existing notion client that was using the previous decorator has been refactored to use the new class approach and the airtable connector was updated to support retry logic as well. Default log handlers were introduced which applies to all instances of the retry handler when it starts, backs off, and gives up.

A generic approach was added to configuring the retry parameters in the CLI and was added to the running number of common configs across all CLI commands.

Omitted CHANGELOG entry as this is mostly just a refactor of the retry code. All other connectors will be updated to support retry in another PR but this helps limit the number of changes to review in this one.

Extra fixes

• Updated local and salesforce source connector to set ingest_doc_cls in a __post_init__ method since this variable can't be serialized.

Testing

Both the airtable and notion ingest tests can be run locally. While they might not pass due to text changes (to be expected when running locally), the process can be viewed in the logs to validate.

Associated issue: #1488

[ryannikolaidis]

so where we were headed with wrapping / raising those custom errors was that one could catch connection errors and re-run outside of the call to the ingest doc. then the code in the ingest doc itself could be somewhat agnostic to retry logic. obviously notion was a bit of a special case since it has tons of nested internal calls...also maybe not all connection errors necessarily make sense to retry? wondering if a: this still makes sense to consider and b: we need to specify some kind of retry-worthy error.

[rbiseck3]

so where we were headed with wrapping / raising those custom errors was that one could catch connection errors and re-run outside of the call to the ingest doc. then the code in the ingest doc itself could be somewhat agnostic to retry logic. obviously notion was a bit of a special case since it has tons of nested internal calls...also maybe not all connection errors necessarily make sense to retry? wondering if a: this still makes sense to consider and b: we need to specify some kind of retry-worthy error.

Some concerns about this, if we move it out of the ingest doc class, then the code that we would be retrying would be the entire get_file method rather than the particular network call that may have failed, adding additional logic that otherwise wouldn't need to be reproduced.

If we're fine with that additional overhead, I can isolate the network calls and wrap that in a specific custom error to indicate it's something that should be retried.

[ryannikolaidis]

so where we were headed with wrapping / raising those custom errors was that one could catch connection errors and re-run outside of the call to the ingest doc. then the code in the ingest doc itself could be somewhat agnostic to retry logic. obviously notion was a bit of a special case since it has tons of nested internal calls...also maybe not all connection errors necessarily make sense to retry? wondering if a: this still makes sense to consider and b: we need to specify some kind of retry-worthy error.

Some concerns about this, if we move it out of the ingest doc class, then the code that we would be retrying would be the entire get_file method rather than the particular network call that may have failed, adding additional logic that otherwise wouldn't need to be reproduced.

If we're fine with that additional overhead, I can isolate the network calls and wrap that in a specific custom error to indicate it's something that should be retried.

yea, totally understood and agreed. I think in a majority of cases that extra logic is going to be nominal. for instance, with the airbyte connector you updated here afaict in get_file the only thing it is doing before the actual query is importing pd. That said, totally get that this isn't always the case...clearly for Notion, what you're doing makes a ton of sense. maybe some simple compromise, do exactly what you're doing, but assuming the user doesn't specify retry then we use the call instead to just raise some custom error for "retry-able" issues? then the caller would have the flexibility to either use what's built in or wrap their own logic?

[ryannikolaidis]

can we can call this max-retries as you had it before? I think this makes it clear it's tied to retry logic

[ryannikolaidis]

max-retry-time? (so it's clear this is specifically about retry

[ryannikolaidis]

depending on sequencing of PRs, just want to make sure this is also captured in documentation at some point.

[ryannikolaidis]

nit on naming otherwise lgtm