-
Notifications
You must be signed in to change notification settings - Fork 249
fix: move RetryCount default to config.Validate() across all drivers #1058
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
base: staging
Are you sure you want to change the base?
Changes from 11 commits
861adcb
8b34343
f3b2014
595d408
48bb82b
38f3ef9
4b6e20f
75336a9
f0b4a29
5b0e0c6
4d1139d
d366f74
3c2bae8
9a754e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,6 @@ import ( | |
| "github.com/datazip-inc/olake/drivers/abstract" | ||
| "github.com/datazip-inc/olake/pkg/jdbc" | ||
| "github.com/datazip-inc/olake/types" | ||
| "github.com/datazip-inc/olake/utils" | ||
| "github.com/datazip-inc/olake/utils/logger" | ||
| "github.com/jmoiron/sqlx" | ||
| "golang.org/x/crypto/ssh" | ||
|
|
@@ -79,7 +78,6 @@ func (d *DB2) Setup(ctx context.Context) error { | |
| } | ||
|
|
||
| d.client = client | ||
| d.config.RetryCount = utils.Ternary(d.config.RetryCount <= 0, 1, d.config.RetryCount+1).(int) | ||
| return nil | ||
|
Comment on lines
-82
to
81
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In olake , by backoff_retry_count we mean is number of retry count for example if user enters 3 backoff retry count than it means 1 main sync + 3 Retry count so can you please make changes according to that.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeahh that makes sense, i have updated all drivers to align with this semantic ( attempts = RetryCount + 1 ) please lmk if this looks good |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,8 +171,6 @@ func (m *Mongo) Setup(ctx context.Context) error { | |
| m.client = conn | ||
| // no need to check from discover if it have cdc support or not | ||
| m.CDCSupport = true | ||
| // check for default backoff count | ||
| m.config.RetryCount = utils.Ternary(m.config.RetryCount == 0, 1, m.config.RetryCount+1).(int) | ||
| pingCtx, cancel := context.WithTimeout(ctx, 1*time.Minute) | ||
|
Comment on lines
173
to
174
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what i was suggesting is instead of removing this part you can let it stay as it is and just remove that check from validate function so it would be single source of truth and just add a small comment over here specifying why we are doing +1 .
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, i actually agree the +1 is intentional, my pr doesnt remove it, it relocates it, retry_count is the no. of retries in RetryOnBackoff expects total attempts, so Maxretries now returns retrycount + 1 ( same behaviour btw ) but the conversation happens at the interface boundary instead of mutating config, and on single source of truth: the line was copy-pasted into 9 drivers setup() method. after my change it exists once i.e constants. DefaultRetryCount + Validate(). also, keeping it in Setup() cant fix the actual bug cleanly imo, like the unset retry_count became 1 attempt (zero retries). That fix requires an inline DefaultRetryCount+1 inside the Ternary, which is exactly what needs the comment you're asking for. In Validate() it reads naturally, and it follows the existing MaxThreads precedent in the same file i think the middle ground would be : i'll add the comment you suggested at the conversion point in MaxRetries() , like lmk if that works
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand what you mean. I think we can handle this in the if c.RetryCount < 0 {
return fmt.Errorf("retry count is required")
}
if c.RetryCount == 0 {
c.RetryCount = constants.DefaultRetryCount
}Apart from that, instead of modifying the value in func RetryOnBackoff(ctx context.Context, attempts int, sleep time.Duration, f func(ctx context.Context) error) (err error) {
// Add 1 because attempts represents the number of retries,
// while the function needs the total number of attempts.
attempts = attempts + 1This way, we are not manipulating the input provided by the user in multiple places, and the retry-count logic stays centralized in And just FYI, in your approach, Mongo is directly calling RetryOnBackoff like this: So it would bypass the changes you're making in MaxRetries and use default values.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also their is one more thing we have changed the scope of this pr that we dont want config file to be manipulated directly so can you once check in entire pr that what we should do for instance in mysql if db name is empty we are setting it to "mysql" but we should fail the sync itself can you once check and made the respective changes |
||
| defer cancel() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,9 +107,8 @@ func (c *Config) Validate() error { | |
| return fmt.Errorf("password is required") | ||
| } | ||
|
|
||
| // Optional database name, default to 'mysql' | ||
| if c.Database == "" { | ||
| c.Database = "mysql" | ||
| return fmt.Errorf("database name is required") | ||
| } | ||
|
Comment on lines
111
to
113
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey sorry for the to and fro, we had an internal discussion and after thinking this particular change might be destructive and of low priority so can you please revert it and dont forget to revert the respective unit test for this fail ? |
||
|
|
||
| // Set default number of threads if not provided | ||
|
|
@@ -118,7 +117,10 @@ func (c *Config) Validate() error { | |
| } | ||
|
|
||
| // Set default retry count if not provided | ||
| if c.RetryCount <= 0 { | ||
| if c.RetryCount < 0 { | ||
| return fmt.Errorf("retry count is required") | ||
| } | ||
| if c.RetryCount == 0 { | ||
| c.RetryCount = constants.DefaultRetryCount // Reasonable default for retries | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,10 @@ func (c *Config) Validate() error { | |
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also can you make the same changes for this one as well as i told you and also do this for other drivers as well |
||
|
|
||
| // Set default retry count | ||
| if c.RetryCount <= 0 { | ||
| if c.RetryCount < 0 { | ||
| return fmt.Errorf("retry count is required") | ||
| } | ||
| if c.RetryCount == 0 { | ||
| c.RetryCount = constants.DefaultRetryCount | ||
| } | ||
|
|
||
|
|
||
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.
Can you please add a space between two if conditions and do it for all the drivers wherever needed?