Skip to content

Commit 89ea7a9

Browse files
RiPontjeffrymorris
RiPont
authored and
jeffrymorris
committed
NCBC-3756: Handle case when server returns empty VBucketMap
Motivation: Rarely, server returns a config with an empty, but non-null VBucketMap Modifications: * If the VBucketMap is null or empty, treat it as if there were no changes * Log if VBucketMap was empty * Only change variables in lock section if there are actual changes Change-Id: I470ec18efa4dab2354265159ef1d46823cb347dc Reviewed-on: https://review.couchbase.org/c/couchbase-net-client/+/207926 Reviewed-by: Jeffry Morris <jeffrymorris@gmail.com> Tested-by: Build Bot <build@couchbase.com>
1 parent fbfd1b0 commit 89ea7a9

File tree

4 files changed

+97
-25
lines changed

4 files changed

+97
-25
lines changed

‎src/Couchbase/Core/Configuration/Server/BucketConfigExtensions.cs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,20 @@ namespace Couchbase.Core.Configuration.Server
99
{
1010
internal static class BucketConfigExtensions
1111
{
12-
public static bool HasVBucketMapChanged(this BucketConfig config, BucketConfig? other)
12+
public static bool HasVBucketMapChanged(this BucketConfig config, BucketConfig? other, out bool emptyVBucketMap)
1313
{
14+
emptyVBucketMap = false;
1415
if (other == null)
1516
{
1617
return true;
1718
}
19+
20+
if (config.VBucketServerMap?.VBucketMap is null or { Length: 0})
21+
{
22+
emptyVBucketMap = true;
23+
return false;
24+
}
25+
1826
return !Equals(config.VBucketServerMap, other.VBucketServerMap);
1927
}
2028

‎src/Couchbase/CouchbaseBucket.cs‎

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ public override async Task ConfigUpdatedAsync(BucketConfig newConfig)
9696
newConfig = _tempConfig;
9797
}
9898

99+
bool anyChanges = false;
99100
if (newConfig.ConfigVersion > CurrentConfig?.ConfigVersion)
100101
{
101102
IKeyMapper? newKeymapper = null;
@@ -122,39 +123,55 @@ public override async Task ConfigUpdatedAsync(BucketConfig newConfig)
122123
newKeymapper =
123124
_vBucketKeyMapperFactory
124125
.Create(newConfig); //force the new revision as were on a new config
126+
anyChanges = true;
125127
}
126128
else
127129
{
128-
if (newConfig.HasVBucketMapChanged(CurrentConfig) || newConfig.IgnoreRev)
130+
if (newConfig.HasVBucketMapChanged(CurrentConfig, out var emptyVBucketMap) || newConfig.IgnoreRev)
129131
{
130132
Logger.LogDebug(LoggingEvents.ConfigEvent,
131133
"Updating VB key mapper for rev#{revision} on {bucketName}",
132134
newConfig.ConfigVersion, Name);
133135
newKeymapper = _vBucketKeyMapperFactory.Create(newConfig);
136+
anyChanges = true;
137+
}
138+
139+
if (emptyVBucketMap)
140+
{
141+
Logger.LogInformation("Encountered bucket config with empty VBucketMap");
134142
}
135143
}
136144
}
137145

138146
//only accept the latest version if the processing was successful
139-
lock (_currentConfigLock)
147+
if (anyChanges)
140148
{
141-
if (newKeymapper is not null)
149+
lock (_currentConfigLock)
142150
{
143-
KeyMapper = newKeymapper;
144-
}
151+
if (newKeymapper is not null)
152+
{
153+
KeyMapper = newKeymapper;
154+
}
145155

146-
if (newNodes is not null)
147-
{
148-
//update the local nodes collection
149-
Nodes.Clear();
150-
foreach (var clusterNode in newNodes)
156+
if (newNodes is not null)
151157
{
152-
Nodes.Add(clusterNode);
158+
//update the local nodes collection
159+
Nodes.Clear();
160+
foreach (var clusterNode in newNodes)
161+
{
162+
Nodes.Add(clusterNode);
163+
}
153164
}
154-
}
155165

156-
CurrentConfig = newConfig;
157-
Logger.LogDebug(LoggingEvents.ConfigEvent,"Current revision for {bucketName} is rev#{revision}", Name, CurrentConfig?.ConfigVersion);
166+
CurrentConfig = newConfig;
167+
Logger.LogDebug(LoggingEvents.ConfigEvent,
168+
"Current revision for {bucketName} is rev#{revision}", Name,
169+
CurrentConfig?.ConfigVersion);
170+
}
171+
}
172+
else
173+
{
174+
Logger.LogDebug(LoggingEvents.ConfigEvent, "BucketConfig processed, but no effective changes applied");
158175
}
159176
}
160177
}

‎tests/Couchbase.UnitTests/Core/Configuration/Server/BucketConfigExtensionTests.cs‎

Lines changed: 54 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public class BucketConfigExtensionTests
2323
public void Test_HasVBucketChanged_OldConfig_IsNull()
2424
{
2525
var newConfig = new BucketConfig();
26-
Assert.True(newConfig.HasVBucketMapChanged(null));
26+
Assert.True(newConfig.HasVBucketMapChanged(null, out _));
2727
}
2828

2929
[Fact]
@@ -48,7 +48,51 @@ public void Test_HasVBucketChanged_VBucketHasChanged()
4848
ServerList = new []{"localhost1", "localhost2"}
4949
}
5050
};
51-
Assert.True(newConfig.HasVBucketMapChanged(oldConfig));
51+
Assert.True(newConfig.HasVBucketMapChanged(oldConfig, out _));
52+
}
53+
54+
[Fact]
55+
public void Test_HasVBucketChanged_VBucketEmpty()
56+
{
57+
// MB-60405
58+
var oldConfig = new BucketConfig
59+
{
60+
VBucketServerMap = new VBucketServerMapDto
61+
{
62+
VBucketMap = new[] { new short[]{1,0}, new short[]{0,1}},
63+
VBucketMapForward = new[] { new short[]{1,0}, new short[]{0,1}},
64+
ServerList = new []{"localhost1", "localhost2"}
65+
}
66+
};
67+
68+
{
69+
var newConfig = new BucketConfig
70+
{
71+
VBucketServerMap = new VBucketServerMapDto
72+
{
73+
VBucketMap = new short[][] { }, //changed, but empty.
74+
VBucketMapForward = new[] { new short[] { 1, 0 }, new short[] { 0, 1 } },
75+
ServerList = new[] { "localhost1", "localhost2" }
76+
}
77+
};
78+
Assert.False(newConfig.HasVBucketMapChanged(oldConfig, out var emptyVBucketMap));
79+
Assert.True(emptyVBucketMap);
80+
}
81+
82+
{
83+
var newConfig = new BucketConfig
84+
{
85+
VBucketServerMap = new VBucketServerMapDto
86+
{
87+
// ReSharper disable once AssignNullToNotNullAttribute
88+
VBucketMap = null, //changed, but missing.
89+
VBucketMapForward = new[] { new short[] { 1, 0 }, new short[] { 0, 1 } },
90+
ServerList = new[] { "localhost1", "localhost2" }
91+
}
92+
};
93+
Assert.False(newConfig.HasVBucketMapChanged(oldConfig, out var emptyVBucketMap));
94+
Assert.True(emptyVBucketMap);
95+
}
5296
}
5397

5498
[Fact]
@@ -73,7 +117,8 @@ public void Test_HasVBucketChanged_VBucketHasNotChanged()
73117
ServerList = new []{"localhost1", "localhost2"}
74118
}
75119
};
76-
Assert.False(newConfig.HasVBucketMapChanged(oldConfig));
120+
Assert.False(newConfig.HasVBucketMapChanged(oldConfig, out var emptyVBucketMap));
121+
Assert.False(emptyVBucketMap);
77122
}
78123

79124
[Fact]
@@ -98,7 +143,8 @@ public void Test_HasVBucketChanged_VBucketMapForward_HasChanged()
98143
ServerList = new []{"localhost1", "localhost2"}
99144
}
100145
};
101-
Assert.True(newConfig.HasVBucketMapChanged(oldConfig));
146+
Assert.True(newConfig.HasVBucketMapChanged(oldConfig, out var emptyVBucketMap));
147+
Assert.False(emptyVBucketMap);
102148
}
103149

104150
[Fact]
@@ -500,18 +546,18 @@ public async Task Test_HasConfigChanges()
500546

501547
var bucket = CreateBucket(configA);
502548
Assert.Equal(configA, bucket.CurrentConfig);
503-
Assert.True(configA.HasVBucketMapChanged(null));
549+
Assert.True(configA.HasVBucketMapChanged(null, out _));
504550
Assert.True(configA.HasClusterNodesChanged(null));
505551

506552
await bucket.ConfigUpdatedAsync(configB);
507553
Assert.Equal(configB, bucket.CurrentConfig);
508-
Assert.True(configB.HasVBucketMapChanged(configA));
554+
Assert.True(configB.HasVBucketMapChanged(configA, out _));
509555
Assert.True(configB.HasClusterNodesChanged(configA));
510556

511557
await bucket.ConfigUpdatedAsync(configC);
512558
Assert.Equal(configC, bucket.CurrentConfig);
513-
Assert.True(configC.HasVBucketMapChanged(configB));
514-
Assert.True(configC.HasVBucketMapChanged(configB));
559+
Assert.True(configC.HasVBucketMapChanged(configB, out _));
560+
Assert.True(configC.HasVBucketMapChanged(configB, out _));
515561
}
516562

517563
CouchbaseBucket CreateBucket(BucketConfig bootstrapConfig)

‎tests/Couchbase.UnitTests/Core/IO/Operations/ClusterMapChangedNotificationTests.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,10 @@ public void When_Replica_Changes_Should_Not_Throw()
8080
var preChange = JsonConvert.DeserializeObject<BucketConfig>(preChangeJson);
8181
var postChange = JsonConvert.DeserializeObject<BucketConfig>(postChangeJson);
8282

83-
var selfCompare = postChange.HasVBucketMapChanged(postChange);
83+
var selfCompare = postChange.HasVBucketMapChanged(postChange, out var emptyVBucketMap);
8484
Assert.False(selfCompare);
85+
Assert.False(emptyVBucketMap);
8586

86-
Assert.True(preChange.HasVBucketMapChanged(postChange));
87+
Assert.True(preChange.HasVBucketMapChanged(postChange, out _));
8788
}
8889
}

0 commit comments

Comments
 (0)