Skip to content

Commit 140868b

Browse files
jadewang-dbJade Wang
andauthored
fix(csharp): disable feature flag cache by default (#358)
## 🥞 Stacked PR Use this [link](https://github.com/adbc-drivers/databricks/pull/358/files) to review incremental changes. - [**stack/disable-config-cache**](#358) [[Files changed](https://github.com/adbc-drivers/databricks/pull/358/files)] --------- ## What's Changed Please fill in a description of the changes here. **This contains breaking changes.** <!-- Remove this line if there are no breaking changes. --> Closes #NNN. --------- Co-authored-by: Jade Wang <jade.wang+data@databricks.com>
1 parent 8ec57f0 commit 140868b

File tree

6 files changed

+87
-16
lines changed

6 files changed

+87
-16
lines changed

csharp/src/DatabricksParameters.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ public class DatabricksParameters : SparkParameters
360360
/// <summary>
361361
/// Whether to enable the feature flag cache for fetching remote configuration from the server.
362362
/// When enabled, the driver fetches feature flags from the Databricks server and merges them with local properties.
363-
/// Default value is true if not specified.
363+
/// Default value is false if not specified.
364364
/// </summary>
365365
public const string FeatureFlagCacheEnabled = "adbc.databricks.feature_flag_cache_enabled";
366366

csharp/src/FeatureFlagCache.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,9 @@ public async Task<IReadOnlyDictionary<string, string>> MergePropertiesWithFeatur
307307

308308
try
309309
{
310-
// Check if feature flag cache is enabled (default: true)
311-
if (localProperties.TryGetValue(DatabricksParameters.FeatureFlagCacheEnabled, out string? enabledStr) &&
312-
bool.TryParse(enabledStr, out bool enabled) &&
310+
// Check if feature flag cache is enabled (default: false)
311+
if (!localProperties.TryGetValue(DatabricksParameters.FeatureFlagCacheEnabled, out string? enabledStr) ||
312+
!bool.TryParse(enabledStr, out bool enabled) ||
313313
!enabled)
314314
{
315315
activity?.AddEvent(new ActivityEvent("feature_flags.skipped",

csharp/src/Telemetry/TelemetryConfiguration.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,9 @@ public sealed class TelemetryConfiguration
8181

8282
/// <summary>
8383
/// Gets or sets whether telemetry is enabled.
84-
/// Default is true.
84+
/// Default is false.
8585
/// </summary>
86-
public bool Enabled { get; set; } = true;
86+
public bool Enabled { get; set; } = false;
8787

8888
/// <summary>
8989
/// Gets or sets the batch size for telemetry metrics.

csharp/test/E2E/FeatureFlagCacheE2ETest.cs

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
using System;
1818
using System.Collections.Generic;
1919
using System.Threading.Tasks;
20+
using Apache.Arrow.Adbc;
2021
using Apache.Arrow.Adbc.Tests;
2122
using Xunit;
2223
using Xunit.Abstractions;
@@ -49,7 +50,7 @@ public async Task TestFeatureFlagCacheInitialization()
4950
Skip.If(string.IsNullOrEmpty(hostName), "Cannot determine host name from test configuration");
5051

5152
// Act - Create a connection which initializes the feature flag cache
52-
using var connection = NewConnection(TestConfiguration);
53+
using var connection = NewConnectionWithFeatureFlagCache();
5354

5455
// Assert - The connection should be created successfully
5556
Assert.NotNull(connection);
@@ -97,8 +98,8 @@ public async Task TestFeatureFlagCacheSharedAcrossConnections()
9798
var cache = FeatureFlagCache.GetInstance();
9899

99100
// Act - Create two connections to the same host
100-
using var connection1 = NewConnection(TestConfiguration);
101-
using var connection2 = NewConnection(TestConfiguration);
101+
using var connection1 = NewConnectionWithFeatureFlagCache();
102+
using var connection2 = NewConnectionWithFeatureFlagCache();
102103

103104
// Assert - Both connections should work properly
104105
Assert.NotNull(connection1);
@@ -134,7 +135,7 @@ public async Task TestFeatureFlagCachePersistsAfterConnectionClose()
134135
OutputHelper?.WriteLine($"[FeatureFlagCacheE2ETest] Initial cache count: {cache.CachedHostCount}");
135136

136137
// Act - Create and close a connection
137-
using (var connection = NewConnection(TestConfiguration))
138+
using (var connection = NewConnectionWithFeatureFlagCache())
138139
{
139140
// Connection is active, cache should have a context for this host
140141
Assert.NotNull(connection);
@@ -176,7 +177,7 @@ public async Task TestFeatureFlagCachePersistsAfterConnectionClose()
176177
public async Task TestConnectionWithFeatureFlagsExecutesQueries()
177178
{
178179
// Arrange
179-
using var connection = NewConnection(TestConfiguration);
180+
using var connection = NewConnectionWithFeatureFlagCache();
180181

181182
// Act - Execute multiple queries to ensure feature flags don't interfere
182183
var queries = new[]
@@ -202,6 +203,18 @@ public async Task TestConnectionWithFeatureFlagsExecutesQueries()
202203
}
203204
}
204205

206+
/// <summary>
207+
/// Creates a connection with feature flag cache explicitly enabled.
208+
/// </summary>
209+
private AdbcConnection NewConnectionWithFeatureFlagCache()
210+
{
211+
var parameters = GetDriverParameters(TestConfiguration);
212+
parameters[DatabricksParameters.FeatureFlagCacheEnabled] = "true";
213+
var driver = new DatabricksDriver();
214+
var database = driver.Open(parameters);
215+
return database.Connect(new Dictionary<string, string>());
216+
}
217+
205218
/// <summary>
206219
/// Gets the normalized host name from test configuration.
207220
/// Strips protocol prefix if present (e.g., "https://host" -> "host").

csharp/test/Unit/FeatureFlagCacheTests.cs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,64 @@ public async Task FeatureFlagContext_BackgroundRefreshError_ContinuesRefreshLoop
567567

568568
#endregion
569569

570+
#region MergePropertiesWithFeatureFlagsAsync Default Behavior Tests
571+
572+
[Fact]
573+
public async Task MergePropertiesWithFeatureFlagsAsync_PropertyNotSet_ReturnsLocalProperties()
574+
{
575+
// Arrange - No FeatureFlagCacheEnabled property set (default: false)
576+
var localProperties = new Dictionary<string, string>
577+
{
578+
["host"] = TestHost,
579+
["some_property"] = "some_value"
580+
};
581+
var cache = FeatureFlagCache.GetInstance();
582+
583+
// Act
584+
var result = await cache.MergePropertiesWithFeatureFlagsAsync(localProperties, DriverVersion);
585+
586+
// Assert - Should return local properties unchanged (feature flags skipped)
587+
Assert.Same(localProperties, result);
588+
}
589+
590+
[Fact]
591+
public async Task MergePropertiesWithFeatureFlagsAsync_PropertySetToFalse_ReturnsLocalProperties()
592+
{
593+
// Arrange - FeatureFlagCacheEnabled explicitly set to false
594+
var localProperties = new Dictionary<string, string>
595+
{
596+
["host"] = TestHost,
597+
[DatabricksParameters.FeatureFlagCacheEnabled] = "false"
598+
};
599+
var cache = FeatureFlagCache.GetInstance();
600+
601+
// Act
602+
var result = await cache.MergePropertiesWithFeatureFlagsAsync(localProperties, DriverVersion);
603+
604+
// Assert - Should return local properties unchanged (feature flags skipped)
605+
Assert.Same(localProperties, result);
606+
}
607+
608+
[Fact]
609+
public async Task MergePropertiesWithFeatureFlagsAsync_PropertySetToInvalidValue_ReturnsLocalProperties()
610+
{
611+
// Arrange - FeatureFlagCacheEnabled set to a non-boolean value
612+
var localProperties = new Dictionary<string, string>
613+
{
614+
["host"] = TestHost,
615+
[DatabricksParameters.FeatureFlagCacheEnabled] = "notabool"
616+
};
617+
var cache = FeatureFlagCache.GetInstance();
618+
619+
// Act
620+
var result = await cache.MergePropertiesWithFeatureFlagsAsync(localProperties, DriverVersion);
621+
622+
// Assert - Should return local properties unchanged (can't parse as bool)
623+
Assert.Same(localProperties, result);
624+
}
625+
626+
#endregion
627+
570628
#region Helper Methods
571629

572630
/// <summary>

csharp/test/Unit/Telemetry/TelemetryConfigurationTests.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public void TelemetryConfiguration_DefaultValues_AreCorrect()
5050
var config = new TelemetryConfiguration();
5151

5252
// Assert
53-
Assert.True(config.Enabled);
53+
Assert.False(config.Enabled);
5454
Assert.Equal(100, config.BatchSize);
5555
Assert.Equal(5000, config.FlushIntervalMs);
5656
Assert.Equal(3, config.MaxRetries);
@@ -106,7 +106,7 @@ public void TelemetryConfiguration_InvalidProperty_UsesDefault()
106106

107107
// Assert - should use defaults for invalid values
108108
Assert.Equal(100, config.BatchSize); // default
109-
Assert.True(config.Enabled); // default
109+
Assert.False(config.Enabled); // default
110110
Assert.Equal(5000, config.FlushIntervalMs); // default
111111
}
112112

@@ -117,7 +117,7 @@ public void TelemetryConfiguration_NullProperties_UsesDefaults()
117117
var config = TelemetryConfiguration.FromProperties(null);
118118

119119
// Assert
120-
Assert.True(config.Enabled);
120+
Assert.False(config.Enabled);
121121
Assert.Equal(100, config.BatchSize);
122122
Assert.Equal(5000, config.FlushIntervalMs);
123123
}
@@ -132,7 +132,7 @@ public void TelemetryConfiguration_EmptyProperties_UsesDefaults()
132132
var config = TelemetryConfiguration.FromProperties(properties);
133133

134134
// Assert
135-
Assert.True(config.Enabled);
135+
Assert.False(config.Enabled);
136136
Assert.Equal(100, config.BatchSize);
137137
Assert.Equal(5000, config.FlushIntervalMs);
138138
}
@@ -187,7 +187,7 @@ public void TelemetryConfiguration_InvalidEnvironmentVariable_UsesDefault()
187187

188188
// Assert - should use defaults for invalid environment variables
189189
Assert.Equal(100, config.BatchSize);
190-
Assert.True(config.Enabled);
190+
Assert.False(config.Enabled);
191191
}
192192

193193
[Fact]

0 commit comments

Comments
 (0)