fix: move RetryCount default to config.Validate() across all drivers

[cotishq]

Description

Fixes #993
Previously, every driver's Setup() method contained an ad-hoc line that both set the default retry count and silently mutated it, incrementing it by 1 when a value was already present:

// old pattern (e.g. postgres.go)
p.config.RetryCount = utils.Ternary(p.config.RetryCount <= 0, 1, p.config.RetryCount+1).(int)

This had two bugs:

1. Off-by-one on user-supplied values: a user who set retry_count: 3 would silently get 4 retries.
2. Wrong place for default logic: defaults belong in Config.Validate(), which is called before Setup(), not inside connection setup code.

This PR removes all those lines and moves the default initialisation into each driver's Config.Validate():

// new pattern (e.g. drivers/postgres/internal/config.go)
if c.RetryCount <= 0 {
    c.RetryCount = constants.DefaultRetryCount
}

constants.DefaultRetryCount = 3 is now consistently used as the fallback across all drivers. The value is set once, early, and never mutated again.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Scenario A - Default retry count ( omitted from config )

Previously, Setup() would have set it to 1.

• Scenario B - User-supplied retry count is respected without mutation -

Added "retry_count": 5 to source.json and re-ran the same sync command. Logs confirmed:

Screenshots or Recordings

https://drive.google.com/file/d/1S1Nzz4CWyiUkyBCyCM5HqSHa2H6rMFZm/view?usp=sharing

Documentation

• Documentation Link: [link to README, olake.io/docs, or olake-docs]
• N/A (bug fix, refactor, or test changes only)

[nayanj98]

@cotishq assigning @saksham-datazip to review your PR

[saksham-datazip]

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.

[cotishq]

yeahh that makes sense, i have updated all drivers to align with this semantic ( attempts = RetryCount + 1 )

please lmk if this looks good

[saksham-datazip]

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 .
if you have some other reasoning for the changes you made than feel free to discuss.

[cotishq]

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

// RetryCount is the number of retries; +1 accounts for the initial attempt,
// since RetryOnBackoff expects total attempts.
return p.config.RetryCount + 1

lmk if that works

[saksham-datazip]

I understand what you mean. I think we can handle this in the Setup() function itself: if RetryCount < 0, we fail the sync, and if RetryCount == 0, we set it to the default retry count.

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 MaxRetries, we can handle the +1 in a single place inside RetryOnBackoff() and add a comment explaining why it is needed:

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 + 1

This way, we are not manipulating the input provided by the user in multiple places, and the retry-count logic stays centralized in RetryOnBackoff().

And just FYI, in your approach, Mongo is directly calling RetryOnBackoff like this:

err = utils.RetryOnBackoff(ctx, m.config.RetryCount, constants.DefaultRetryTimeout, func(ctx context.Context) error {
    chunksArray, retryErr = m.splitChunks(ctx, collection, stream, storageSize)
    return retryErr
})

So it would bypass the changes you're making in MaxRetries and use default values.

[saksham-datazip]

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

[cotishq]

Thanks for the review @saksham-datazip

I agree with your suggestion to make RetryOnBackoff the single source of truth for retry handling. Here is what has been updated:

1. Centralized Retries Handling (utils.RetryOnBackoff):
   • Added attempts = attempts + 1 inside utils.RetryOnBackoff() with explanatory comments.
   • RetryCount = 3 now represents 3 retries (4 total attempts) across all drivers, avoiding duplicate +1 logic in Setup() or MaxRetries().
2. Validation (config.Validate()):
   • Updated Validate() so c.RetryCount < 0 returns an explicit error to fail the sync early, while c.RetryCount == 0 assigns constants.DefaultRetryCount.
3. Rebase & Cleanup:
   • Resolved rebase conflicts against latest upstream/staging.
   • Verified that all unit tests across drivers pass cleanly.

lmk if this looks good, or we have to make more changes

[saksham-datazip]

@cotishq Just fyi we avoid using force push or rebase in olake so please dont do it from now onwards

[saksham-datazip]

@cotishq I think there may be some misunderstanding regarding my last comment:

1->As mentioned, the scope of this PR has changed, and we now want the ConfigMap to remain unaffected, so could you please go through my previous comment once again before making any changes?
2->If anything is unclear, please feel free to ask me directly rather than making assumptions.
3->Regarding Testing just running unit tests is not enough you have to run sync as well and think about edge cases w.r.t. changes you have made.
Also, if you're making a change in one place, please check whether the same change should be applied elsewhere and make the corresponding changes where applicable.

[saksham-datazip]

I told you to fail the sync for c.RetryCount<0 case right ?Please do it for all the drivers.

[cotishq]

@cotishq Just fyi we avoid using force push or rebase in olake so please dont do it from now onwards

ok i'll keep this in mind

[cotishq]

@saksham-datazip,

Apologies for the previous misunderstanding, i just took that in wrong way but i have fixed the things now like this:

1. Negative RetryCount < 0 Fails Sync:

   • Updated config.Validate() across all drivers (mongodb, mysql, postgres, db2, mssql, oracle, s3, kafka) so RetryCount < 0 returns an error ("retry count is required"), while RetryCount == 0 defaults to constants.DefaultRetryCount.

2. Config Mutation Audit:

   • Removed direct config field mutation defaults (e.g. in MySQL, empty c.Database no longer sets c.Database = "mysql", but fails validation requiring database name).

3. Tests:

   • Added unit test cases across all driver config_test.go files verifying that negative retry counts and missing required fields return errors.

lmk, if this looks good to you, comfortable to address more suggestions too

[saksham-datazip]

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 ?

[saksham-datazip]

Can you please add a space between two if conditions and do it for all the drivers wherever needed?

[saksham-datazip]

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

[saksham-datazip]

@cotishq I’ve reviewed your PR. Please make the necessary changes accordingly. If there’s no response or progress on the requested changes within two days, we’ll close the PR.

[cotishq]

@cotishq I’ve reviewed your PR. Please make the necessary changes accordingly. If there’s no response or progress on the requested changes within two days, we’ll close the PR.

yess, making the changes, will update you shortly

[cotishq]

@saksham-datazip i have addressed all the suggestions, ptal and lmk i we have to do more changes