Exporter: Add retries for Search, ReadContext and Import operations when importing the resource
#3202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…en importing the resource Under a very high load, Databricks backend may not answer on time, or return specific errors, so it makes sense to retry operation few times. This PR uses "naive" implementation, I need to play a bit more with `retries` package before adopting it
|
@mgyucht @tanmay-db It would be really useful to get this merged before the release... |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3202 +/- ##
==========================================
+ Coverage 83.57% 83.58% +0.01%
==========================================
Files 168 168
Lines 15021 15063 +42
==========================================
+ Hits 12554 12591 +37
- Misses 1729 1733 +4
- Partials 738 739 +1
|
|
looking |
tanmay-db
reviewed
Feb 1, 2024
alexott
force-pushed
the
exporter-retries-under-high-load
branch
from
b126bc0
to
6fec3ff
Compare
alexott
changed the title
Search, ReadContext and Import operations when importing the resourceSearch, ReadContext and Import operations when importing the resource
alexott
changed the title
Search, ReadContext and Import operations when importing the resourceSearch, ReadContext and Import operations when importing the resource
tanmay-db
reviewed
Feb 2, 2024
| @@ -758,7 +758,7 @@ func (ic *importContext) generateAndWriteResources(sh *os.File) { | |||
| for i, r := range resources { | |||
| ic.waitGroup.Add(1) | |||
| resourcesChan <- r | |||
| if i%50 == 0 { | |||
| if i%500 == 0 { | |||
There was a problem hiding this comment.
I am guessing this is because there are too many items in logs so we want to minimise them?
There was a problem hiding this comment.
yes, it's just too much
| return false | ||
| } | ||
|
|
||
| func runWithRetries[ERR any](runFunc func() ERR, msg string) ERR { |
There was a problem hiding this comment.
Rest looks good to me, only concern is that we can use https://github.com/databricks/databricks-sdk-go/blob/main/retries/retries.go here otherwise, we would have to maintain this retry mechanism separately.
If the change required is urgent and using retries would require major refactoring, then we can go ahead with this. What do you think? @mgyucht
There was a problem hiding this comment.
retries package is a convenience, not a core part of the SDK. The retries package does not expose anything to e.g. print when a retry is taking place or print a specific message after including the total number of retries that have happened, so if @alexott needs these, it's perfectly fine for him to implement his own retry logic.
There was a problem hiding this comment.
Though @alexott curious if we added things like WithMaxRetries or WithRetryOnErrorSubstrings if that would satisfy your need.
There was a problem hiding this comment.
yep, let discuss if we can make something generic...
tanmay-db
added a commit
that referenced
this pull request
Feb 5, 2024
### New Features and Improvements * Exporter: timestamps are now added to log entries ([#3146](#3146)). * Validate metastore id for databricks_grant and databricks_grants resources ([#3159](#3159)). * Exporter: Skip emitting of clusters that come from more cluster sources ([#3161](#3161)). * Fix typo in docs ([#3166](#3166)). * Migrate cluster schema to use the go-sdk struct ([#3076](#3076)). * Introduce Generic Settings Resource ([#2997](#2997)). * Update actions/setup-go to v5 ([#3154](#3154)). * Change default branch from `master` to `main` ([#3174](#3174)). * Add .codegen.json configuration ([#3180](#3180)). * Exporter: performance improvements for big workspaces ([#3167](#3167)). * update ([#3192](#3192)). * Exporter: fix generation of cluster policy resources ([#3185](#3185)). * Fix unit test ([#3201](#3201)). * Suppress diff should apply to new fields added in the same chained call to CustomizableSchema ([#3200](#3200)). * Various documentation updates ([#3198](#3198)). * Use common.Resource consistently throughout the provider ([#3193](#3193)). * Extending customizable schema with `AtLeastOneOf`, `ExactlyOneOf`, `RequiredWith` ([#3182](#3182)). * Fix `databricks_connection` regression when creating without owner ([#3186](#3186)). * add test code for job task order ([#3183](#3183)). * Allow using empty strings as job parameters ([#3158](#3158)). * Fix notebook parameters in acceptance test ([#3205](#3205)). * Exporter: Add retries for `Search`, `ReadContext` and `Import` operations when importing the resource ([#3202](#3202)). * Fixed updating owners for UC resources ([#3189](#3189)). * Adds `databricks_volumes` as data source ([#3150](#3150)). ### Documentation Changes ### Exporter ### Internal Changes
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 6, 2024
* Release v1.35.1 ### New Features and Improvements * Exporter: timestamps are now added to log entries ([#3146](#3146)). * Validate metastore id for databricks_grant and databricks_grants resources ([#3159](#3159)). * Exporter: Skip emitting of clusters that come from more cluster sources ([#3161](#3161)). * Fix typo in docs ([#3166](#3166)). * Migrate cluster schema to use the go-sdk struct ([#3076](#3076)). * Introduce Generic Settings Resource ([#2997](#2997)). * Update actions/setup-go to v5 ([#3154](#3154)). * Change default branch from `master` to `main` ([#3174](#3174)). * Add .codegen.json configuration ([#3180](#3180)). * Exporter: performance improvements for big workspaces ([#3167](#3167)). * update ([#3192](#3192)). * Exporter: fix generation of cluster policy resources ([#3185](#3185)). * Fix unit test ([#3201](#3201)). * Suppress diff should apply to new fields added in the same chained call to CustomizableSchema ([#3200](#3200)). * Various documentation updates ([#3198](#3198)). * Use common.Resource consistently throughout the provider ([#3193](#3193)). * Extending customizable schema with `AtLeastOneOf`, `ExactlyOneOf`, `RequiredWith` ([#3182](#3182)). * Fix `databricks_connection` regression when creating without owner ([#3186](#3186)). * add test code for job task order ([#3183](#3183)). * Allow using empty strings as job parameters ([#3158](#3158)). * Fix notebook parameters in acceptance test ([#3205](#3205)). * Exporter: Add retries for `Search`, `ReadContext` and `Import` operations when importing the resource ([#3202](#3202)). * Fixed updating owners for UC resources ([#3189](#3189)). * Adds `databricks_volumes` as data source ([#3150](#3150)). ### Documentation Changes ### Exporter ### Internal Changes * upd * readable * upd * upd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Under a very high load, Databricks backend may not answer on time, or return specific errors, so it makes sense to retry operation few times.
This PR uses "naive" implementation, I need to play a bit more with
retriespackage before adopting itTests
make testrun locallydocs/folderinternal/acceptance