Skip to content

Commit d892736

Browse files
NaluTripicianCopilotkirankumarkolli
authored
[FaultInjection] FaultInjection: Refactors code quality improvements (#5678)
## Summary This PR implements code quality improvements for the FaultInjection library. ### Changes **1. Mark `enabled` fields as `volatile` (#5663)** - Added `volatile` keyword to `enabled` fields in `FaultInjectionRule`, `FaultInjectionServerErrorRule`, `FaultInjectionConnectionErrorRule`, and `FaultInjectionCustomServerErrorRule`. - These fields are read/written from multiple threads via `Enable()`/`Disable()` methods. Without `volatile`, changes may not be visible to other threads due to CPU cache coherence. **2. Add null guards in `FaultInjectionEndpointBuilder` (#5666)** - Added `ArgumentNullException` guards for `databaseName`, `containerName`, and `feedRange` in the constructor. - Previously, null values would result in deferred errors rather than clear exceptions at construction time. **3. Throw when `WithDelay()` called on non-delay error type (#5667)** - Changed `WithDelay()` from silently ignoring the delay for non-applicable error types to throwing `InvalidOperationException`. - Only `SendDelay`, `ResponseDelay`, and `ConnectionDelay` support delays. Calling `WithDelay()` on other types (e.g., `Gone`) now throws to make misconfiguration explicit. **4. Implement `CanLimitToPartition` for metadata operations (#5669)** - Replaced the TODO stub with actual implementation. - Returns `false` for metadata operation types (`MetadataContainer`, `MetadataDatabaseAccount`, `MetadataPartitionKeyRange`, `MetadataRefreshAddresses`, `MetadataQueryPlan`) since these are account/collection-level operations that cannot be targeted to specific partitions. ### Testing - Build verified locally with `dotnet build` Parent tracking issue: #5652 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Kiran Kumar Kolli <kirankk@microsoft.com>
1 parent 7126846 commit d892736

9 files changed

Lines changed: 31 additions & 38 deletions

‎Microsoft.Azure.Cosmos/FaultInjection/src/FaultInjectionEndpointBuilder.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,19 @@ public sealed class FaultInjectionEndpointBuilder
2424
/// <param name="feedRange">the <see cref="FeedRange"/>.</param>
2525
public FaultInjectionEndpointBuilder(string databaseName, string containerName, FeedRange feedRange)
2626
{
27+
if (string.IsNullOrEmpty(databaseName))
28+
{
29+
throw new ArgumentNullException(nameof(databaseName));
30+
}
31+
32+
if (string.IsNullOrEmpty(containerName))
33+
{
34+
throw new ArgumentNullException(nameof(containerName));
35+
}
36+
2737
this.databaseName = databaseName;
2838
this.containerName = containerName;
29-
this.feedRange = feedRange;
39+
this.feedRange = feedRange ?? throw new ArgumentNullException(nameof(feedRange));
3040
}
3141

3242
/// <summary>

‎Microsoft.Azure.Cosmos/FaultInjection/src/FaultInjectionRule.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
//------------------------------------------------------------
1+
//------------------------------------------------------------
22
// Copyright (c) Microsoft Corporation. All rights reserved.
33
//------------------------------------------------------------
44
namespace Microsoft.Azure.Cosmos.FaultInjection
@@ -17,7 +17,7 @@ public sealed class FaultInjectionRule
1717
private readonly TimeSpan duration;
1818
private readonly TimeSpan startDelay;
1919
private readonly int hitLimit;
20-
private bool enabled;
20+
private volatile bool enabled;
2121
private IFaultInjectionRuleInternal? effectiveRule;
2222

2323
/// <summary>

‎Microsoft.Azure.Cosmos/FaultInjection/src/FaultInjectionServerErrorResultBuilder.cs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,17 @@ public FaultInjectionServerErrorResultBuilder WithTimes(int times)
5252
/// <returns>The current <see cref="FaultInjectionServerErrorResultBuilder"/>.</returns>
5353
public FaultInjectionServerErrorResultBuilder WithDelay(TimeSpan delay)
5454
{
55-
if ( this.serverErrorType == FaultInjectionServerErrorType.SendDelay
56-
|| this.serverErrorType == FaultInjectionServerErrorType.ResponseDelay
57-
|| this.serverErrorType == FaultInjectionServerErrorType.ConnectionDelay)
55+
if (this.serverErrorType != FaultInjectionServerErrorType.SendDelay
56+
&& this.serverErrorType != FaultInjectionServerErrorType.ResponseDelay
57+
&& this.serverErrorType != FaultInjectionServerErrorType.ConnectionDelay)
5858
{
59-
this.delay = delay;
60-
this.isDelaySet = true;
59+
throw new InvalidOperationException(
60+
$"Delay is not applicable for server error type '{this.serverErrorType}'. " +
61+
$"Delay can only be set for SendDelay, ResponseDelay, or ConnectionDelay.");
6162
}
63+
64+
this.delay = delay;
65+
this.isDelaySet = true;
6266
return this;
6367
}
6468

‎Microsoft.Azure.Cosmos/FaultInjection/src/implementation/FaultInjectionConnectionErrorRule.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
//------------------------------------------------------------
1+
//------------------------------------------------------------
22
// Copyright (c) Microsoft Corporation. All rights reserved.
33
//------------------------------------------------------------
44
namespace Microsoft.Azure.Cosmos.FaultInjection
@@ -17,7 +17,7 @@ public class FaultInjectionConnectionErrorRule : IFaultInjectionRuleInternal
1717
private readonly FaultInjectionConnectionErrorResult result;
1818

1919
private long hitCount;
20-
private bool enabled;
20+
private volatile bool enabled;
2121

2222
public FaultInjectionConnectionErrorRule(
2323
String id,

‎Microsoft.Azure.Cosmos/FaultInjection/src/implementation/FaultInjectionCustomServerErrorRule.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ internal class FaultInjectionCustomServerErrorRule : IFaultInjectionRuleInternal
2828

2929
private long hitCount;
3030
private long evaluationCount;
31-
private bool enabled;
31+
private volatile bool enabled;
3232

3333
public FaultInjectionCustomServerErrorRule(
3434
string id,

‎Microsoft.Azure.Cosmos/FaultInjection/src/implementation/FaultInjectionRuleProcessor.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,12 @@ private bool CanErrorLimitToOperation(FaultInjectionServerErrorType errorType)
328328

329329
private bool CanLimitToPartition(FaultInjectionCondition faultInjectionCondition)
330330
{
331-
// Some operations can be targeted for a certain partition while some can not (for example metadata requests)
332-
//TODO: Implement metadata operations
333331
if (faultInjectionCondition == null)
334332
{
333+
return true;
335334
}
336-
return true;
335+
336+
return !faultInjectionCondition.IsMetadataOperationType();
337337
}
338338

339339
private OperationType GetEffectiveOperationType(FaultInjectionOperationType faultInjectionOperationType)

‎Microsoft.Azure.Cosmos/FaultInjection/src/implementation/FaultInjectionServerErrorRule.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
//------------------------------------------------------------
1+
//------------------------------------------------------------
22
// Copyright (c) Microsoft Corporation. All rights reserved.
33
//------------------------------------------------------------
44
namespace Microsoft.Azure.Cosmos.FaultInjection
@@ -25,7 +25,7 @@ internal class FaultInjectionServerErrorRule : IFaultInjectionRuleInternal
2525

2626
private long hitCount;
2727
private long evaluationCount;
28-
private bool enabled;
28+
private volatile bool enabled;
2929

3030
public FaultInjectionServerErrorRule(
3131
string id,

‎Microsoft.Azure.Cosmos/FaultInjection/tests/FaultInjectionDirectModeTests.cs‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -812,7 +812,6 @@ public async Task FaultInjectionServerErrorRule_ServerTimeout()
812812
.Build(),
813813
result:
814814
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.Timeout)
815-
.WithDelay(TimeSpan.FromSeconds(6))
816815
.WithTimes(1)
817816
.Build())
818817
.WithDuration(TimeSpan.FromMinutes(5))
@@ -842,9 +841,6 @@ public async Task FaultInjectionServerErrorRule_ServerTimeout()
842841

843842
timeoutRule.Enable();
844843

845-
ValueStopwatch stopwatch = ValueStopwatch.StartNew();
846-
TimeSpan elapsed;
847-
848844
try
849845
{
850846
await this.fiContainer.ReadItemAsync<FaultInjectionTestObject>(
@@ -859,10 +855,6 @@ await this.fiContainer.ReadItemAsync<FaultInjectionTestObject>(
859855
timeoutRule);
860856
}
861857

862-
elapsed = stopwatch.Elapsed;
863-
stopwatch.Stop();
864-
865-
Assert.IsTrue(elapsed.TotalSeconds >= 6);
866858
this.ValidateHitCount(timeoutRule, 1);
867859
}
868860
finally

‎Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemIntegrationTests.cs‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
namespace Microsoft.Azure.Cosmos.SDK.EmulatorTests
1+
namespace Microsoft.Azure.Cosmos.SDK.EmulatorTests
22
{
33
using System;
44
using System.Collections.Generic;
@@ -498,7 +498,6 @@ public async Task ReadItemAsync_WithCircuitBreakerEnabledAndSingleMasterAccountA
498498
.Build(),
499499
result:
500500
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
501-
.WithDelay(TimeSpan.FromMilliseconds(10))
502501
.Build())
503502
.Build();
504503

@@ -627,7 +626,6 @@ public async Task ReadItemAsync_WithCircuitBreakerEnabledAndTimeoutCounterOverwr
627626
.Build(),
628627
result:
629628
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
630-
.WithDelay(TimeSpan.FromMilliseconds(10))
631629
.Build())
632630
.Build();
633631

@@ -750,7 +748,6 @@ public async Task ReadItemAsync_WithCircuitBreakerEnabledAndSingleMasterAccountA
750748
.Build(),
751749
result:
752750
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
753-
.WithDelay(TimeSpan.FromMilliseconds(10))
754751
.Build())
755752
.Build();
756753

@@ -764,7 +761,6 @@ public async Task ReadItemAsync_WithCircuitBreakerEnabledAndSingleMasterAccountA
764761
.Build(),
765762
result:
766763
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
767-
.WithDelay(TimeSpan.FromMilliseconds(10))
768764
.Build())
769765
.Build();
770766

@@ -914,7 +910,6 @@ public async Task ReadItemAsync_WithNoPreferredRegionsAndCircuitBreakerEnabledAn
914910
.Build(),
915911
result:
916912
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
917-
.WithDelay(TimeSpan.FromMilliseconds(10))
918913
.Build())
919914
.Build();
920915

@@ -1030,7 +1025,6 @@ public async Task ReadItemAsync_WithCircuitBreakerDisabledAndSingleMasterAccount
10301025
.Build(),
10311026
result:
10321027
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1033-
.WithDelay(TimeSpan.FromMilliseconds(10))
10341028
.Build())
10351029
.Build();
10361030

@@ -1122,7 +1116,6 @@ public async Task CreateItemAsync_WithCircuitBreakerEnabledAndSingleMasterAccoun
11221116
.Build(),
11231117
result:
11241118
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1125-
.WithDelay(TimeSpan.FromMilliseconds(10))
11261119
.Build())
11271120
.Build();
11281121

@@ -1209,7 +1202,6 @@ public async Task CreateItemAsync_WithCircuitBreakerEnabledAndMultiMasterAccount
12091202
.Build(),
12101203
result:
12111204
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1212-
.WithDelay(TimeSpan.FromMilliseconds(10))
12131205
.Build())
12141206
.Build();
12151207

@@ -1333,7 +1325,6 @@ public async Task CreateAndReadItemAsync_WithCircuitBreakerEnabledAndMultiMaster
13331325
.Build(),
13341326
result:
13351327
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1336-
.WithDelay(TimeSpan.FromMilliseconds(2))
13371328
.Build())
13381329
.Build();
13391330

@@ -1347,7 +1338,6 @@ public async Task CreateAndReadItemAsync_WithCircuitBreakerEnabledAndMultiMaster
13471338
.Build(),
13481339
result:
13491340
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1350-
.WithDelay(TimeSpan.FromMilliseconds(2))
13511341
.Build())
13521342
.Build();
13531343

@@ -1596,7 +1586,6 @@ public async Task ReadItemAsync_WithPPAFDynamicOverride_ShouldEnableOrDisablePPA
15961586
.Build(),
15971587
result:
15981588
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1599-
.WithDelay(TimeSpan.FromMilliseconds(10))
16001589
.Build())
16011590
.Build();
16021591

@@ -1957,7 +1946,6 @@ public async Task ReadItemAsync_WithThinClientCircuitBreakerEnabledAndSingleMast
19571946
.Build(),
19581947
result:
19591948
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
1960-
.WithDelay(TimeSpan.FromMilliseconds(10))
19611949
.Build())
19621950
.Build();
19631951

@@ -2085,7 +2073,6 @@ public async Task CreateItemAsync_WithThinClientEnabledAndCircuitBreakerEnabledA
20852073
.Build(),
20862074
result:
20872075
FaultInjectionResultBuilder.GetResultBuilder(FaultInjectionServerErrorType.ServiceUnavailable)
2088-
.WithDelay(TimeSpan.FromMilliseconds(10))
20892076
.Build())
20902077
.Build();
20912078

0 commit comments

Comments
 (0)