Skip to content

Commit 2922c71

Browse files
fix(csharp): use -1 as default for max_retries to denote not set
Change DefaultMaxRetries from 0 to -1 so that -1 clearly means "not set" and 0 can mean "no retries at all" if needed. Guard checks updated from > 0 to >= 0 accordingly. Co-authored-by: Isaac
1 parent 334b90e commit 2922c71

File tree

4 files changed

+12
-12
lines changed

4 files changed

+12
-12
lines changed

csharp/src/DatabricksParameters.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ public class DatabricksParameters : SparkParameters
5353

5454
/// <summary>
5555
/// Maximum number of retry attempts for CloudFetch downloads.
56-
/// Default value is 0 (no limit, use timeout only) if not specified.
57-
/// When set, the retry loop exits if either this count or the timeout is reached.
56+
/// Default value is -1 (not set, use timeout only).
57+
/// When set to a non-negative value, the retry loop exits if either this count or the timeout is reached.
5858
/// </summary>
5959
public const string CloudFetchMaxRetries = "adbc.databricks.cloudfetch.max_retries";
6060

csharp/src/Reader/CloudFetch/CloudFetchConfiguration.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ internal sealed class CloudFetchConfiguration
3333
internal const int DefaultPrefetchCount = 2;
3434
internal const int DefaultMemoryBufferSizeMB = 200;
3535
internal const int DefaultTimeoutMinutes = 5;
36-
internal const int DefaultMaxRetries = 0; // 0 = no limit (use timeout only)
36+
internal const int DefaultMaxRetries = -1; // -1 = not set (use timeout only)
3737
internal const int DefaultRetryTimeoutSeconds = 300; // 5 minutes
3838
internal const int DefaultRetryDelayMs = 500;
3939
internal const int DefaultMaxUrlRefreshAttempts = 3;
@@ -61,8 +61,8 @@ internal sealed class CloudFetchConfiguration
6161

6262
/// <summary>
6363
/// Maximum retry attempts for failed downloads.
64-
/// 0 means no limit (use timeout only). When set, the retry loop exits
65-
/// if either this count or the timeout is reached.
64+
/// -1 means not set (use timeout only). When set to a non-negative value,
65+
/// the retry loop exits if either this count or the timeout is reached.
6666
/// </summary>
6767
public int MaxRetries { get; set; } = DefaultMaxRetries;
6868

@@ -133,7 +133,7 @@ public static CloudFetchConfiguration FromProperties(
133133
PrefetchCount = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchPrefetchCount, DefaultPrefetchCount),
134134
MemoryBufferSizeMB = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchMemoryBufferSize, DefaultMemoryBufferSizeMB),
135135
TimeoutMinutes = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchTimeoutMinutes, DefaultTimeoutMinutes),
136-
MaxRetries = properties.TryGetValue(DatabricksParameters.CloudFetchMaxRetries, out string? maxRetriesStr) && int.TryParse(maxRetriesStr, out int maxRetries) && maxRetries > 0 ? maxRetries : DefaultMaxRetries,
136+
MaxRetries = properties.TryGetValue(DatabricksParameters.CloudFetchMaxRetries, out string? maxRetriesStr) && int.TryParse(maxRetriesStr, out int maxRetries) && maxRetries >= 0 ? maxRetries : DefaultMaxRetries,
137137
RetryTimeoutSeconds = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchRetryTimeoutSeconds, DefaultRetryTimeoutSeconds),
138138
RetryDelayMs = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchRetryDelayMs, DefaultRetryDelayMs),
139139
MaxUrlRefreshAttempts = PropertyHelper.GetPositiveIntPropertyWithValidation(properties, DatabricksParameters.CloudFetchMaxUrlRefreshAttempts, DefaultMaxUrlRefreshAttempts),

csharp/src/Reader/CloudFetch/CloudFetchDownloader.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ await _activityTracer.TraceActivityAsync(async activity =>
503503
while (!cancellationToken.IsCancellationRequested)
504504
{
505505
// Check if we've exceeded the max retry count (if set)
506-
if (_maxRetries > 0 && attemptCount >= _maxRetries)
506+
if (_maxRetries >= 0 && attemptCount >= _maxRetries)
507507
{
508508
activity?.AddEvent("cloudfetch.download_max_retries_exceeded", [
509509
new("offset", downloadResult.StartRowOffset),
@@ -645,7 +645,7 @@ await _activityTracer.TraceActivityAsync(async activity =>
645645

646646
// Release the memory we acquired
647647
_memoryManager.ReleaseMemory(size);
648-
string retryLimits = _maxRetries > 0
648+
string retryLimits = _maxRetries >= 0
649649
? $"max_retries: {_maxRetries}, timeout: {_retryTimeoutSeconds}s"
650650
: $"timeout: {_retryTimeoutSeconds}s";
651651
throw new InvalidOperationException(

csharp/test/E2E/CloudFetch/CloudFetchDownloaderTest.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ public async Task DownloadFileAsync_ProcessesFile_AndAddsToResultQueue()
178178
_mockResultFetcher.Object,
179179
1, // maxParallelDownloads
180180
false, // isLz4Compressed
181-
maxRetries: 0,
181+
maxRetries: -1,
182182
retryTimeoutSeconds: 5,
183183
retryDelayMs: 10);
184184

@@ -263,7 +263,7 @@ public async Task DownloadFileAsync_HandlesHttpError_AndSetsFailedOnDownloadResu
263263
_mockResultFetcher.Object,
264264
1, // maxParallelDownloads
265265
false, // isLz4Compressed
266-
maxRetries: 0,
266+
maxRetries: -1,
267267
retryTimeoutSeconds: 1,
268268
retryDelayMs: 10);
269269

@@ -353,7 +353,7 @@ public async Task DownloadFileAsync_WithError_StopsProcessingRemainingFiles()
353353
_mockResultFetcher.Object,
354354
1, // maxParallelDownloads
355355
false, // isLz4Compressed
356-
maxRetries: 0,
356+
maxRetries: -1,
357357
retryTimeoutSeconds: 1,
358358
retryDelayMs: 10);
359359

@@ -687,7 +687,7 @@ public async Task DownloadFileAsync_RefreshesExpiredUrl_WhenHttpErrorOccurs()
687687
_mockResultFetcher.Object,
688688
1, // maxParallelDownloads
689689
false, // isLz4Compressed
690-
maxRetries: 0,
690+
maxRetries: -1,
691691
retryTimeoutSeconds: 5,
692692
retryDelayMs: 10);
693693

0 commit comments

Comments
 (0)