Skip to content

Commit 72111db

Browse files
author
Ignacio Alonso Battaglia
committed
Merged PR 739397: Rollback: Do not lie about cause of PlaceFailures
Rollback: Do not lie about cause of PlaceFailures
1 parent fad2fc1 commit 72111db

File tree

5 files changed

+9
-43
lines changed

5 files changed

+9
-43
lines changed

‎Public/Src/Cache/ContentStore/Distributed/ContentLocations/ContentHashWithSizeAndLocations.cs‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,6 @@ public class ContentHashWithSizeAndLocations : IEquatable<ContentHashWithSizeAnd
6363
/// </summary>
6464
public GetBulkOrigin? Origin { get; set; }
6565

66-
/// <summary>
67-
/// True if some locations were subtracted from the original list
68-
/// This happens because in order not to repeat our failures we remove locations that we already tried
69-
/// For example, if we have 3 locations in LLS and we fail to get the content from all of them, we will remove them from the GCS result list
70-
/// </summary>
71-
public bool LocationSubtracted { get; set; } = false;
72-
7366
/// <nodoc />
7467
public ContentHashWithSizeAndLocations(ContentHash contentHash, long size = -1)
7568
{
@@ -84,17 +77,14 @@ public ContentHashWithSizeAndLocations(
8477
IReadOnlyList<MachineLocation> locations,
8578
ContentLocationEntry? entry = null,
8679
IReadOnlyList<MachineLocation>? filteredOutLocations = null,
87-
GetBulkOrigin? origin = null,
88-
bool locationSubtracted = false
89-
)
80+
GetBulkOrigin? origin = null)
9081
{
9182
ContentHash = contentHash;
9283
Size = size;
9384
Locations = locations;
9485
Entry = entry;
9586
FilteredOutInactiveMachineLocations = filteredOutLocations;
9687
Origin = origin;
97-
LocationSubtracted = locationSubtracted;
9888
}
9989

10090
/// <summary>

‎Public/Src/Cache/ContentStore/Distributed/ContentLocations/GetBulkLocationsResult.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,7 @@ private static ContentHashWithSizeAndLocations Subtract(ContentHashWithSizeAndLo
166166
Contract.Requires(left.Size == -1 || right.Size == -1 || right.Size == left.Size);
167167

168168
var finalList = (left.Locations ?? Enumerable.Empty<MachineLocation>()).Except(right.Locations ?? Enumerable.Empty<MachineLocation>());
169-
var locationSubtracted = left.Locations != null && finalList.Count() < left.Locations.Count;
170-
return new ContentHashWithSizeAndLocations(left.ContentHash, Math.Max(left.Size, right.Size), finalList.ToList(), locationSubtracted: locationSubtracted);
169+
return new ContentHashWithSizeAndLocations(left.ContentHash, Math.Max(left.Size, right.Size), finalList.ToList());
171170
}
172171

173172
/// <summary>

‎Public/Src/Cache/ContentStore/Distributed/Sessions/DistributedContentSession.cs‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -878,18 +878,6 @@ private bool CanCopyContentHash(
878878
return false;
879879
}
880880

881-
if (!result.Locations.Any() && result.LocationSubtracted)
882-
{
883-
message = $"Replicas in LLS content tracker for hash {result.ContentHash.ToShortString()} but failed to copy all";
884-
return false;
885-
}
886-
887-
if (!result.Locations.Any() && result.FilteredOutInactiveMachineLocations != null && result.FilteredOutInactiveMachineLocations.Any())
888-
{
889-
message = $"Filtered all locations for {result.ContentHash.ToShortString()} because they were inactive machines ";
890-
return false;
891-
}
892-
893881
if (!result.Locations.Any())
894882
{
895883
message = $"No replicas currently exist in content tracker for hash {result.ContentHash.ToShortString()}";

‎Public/Src/Cache/ContentStore/Interfaces/Utils/Workflows.cs‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,7 @@ public static async Task<IEnumerable<Task<Indexed<TResult>>>> RunWithFallback<TS
7373
Indexed<TResult> result = await resultTask;
7474

7575
int originalIndex = indexedFailures[result.Index].Index;
76-
77-
// In case the fallback function failed, we want to add the original failure to the results
78-
var unifiedResult = !isSuccessFunc(result.Item) ? indexedFailures[result.Index] : result.Item.WithIndex(originalIndex);
79-
fixedFallbackResults.Add(unifiedResult);
76+
fixedFallbackResults.Add(result.Item.WithIndex(originalIndex));
8077
}
8178

8279
// Merge original successful results with fallback results
@@ -129,10 +126,7 @@ Func<TResult, bool> isSuccessFunc
129126
Indexed<TResult> result = await resultTask;
130127

131128
int originalIndex = indexedFailures[result.Index].Index;
132-
133-
// In case the fallback function failed, we want to add the original failure to the results
134-
var unifiedResult = !isSuccessFunc(result.Item) ? indexedFailures[result.Index] : result.Item.WithIndex(originalIndex);
135-
fixedFallbackResults.Add(unifiedResult);
129+
fixedFallbackResults.Add(result.Item.WithIndex(originalIndex));
136130
}
137131

138132
// Merge original successful results with fallback results
@@ -187,10 +181,7 @@ Func<TResult, bool> isSuccessFunc
187181
Indexed<TResult> result = await resultTask;
188182

189183
int originalIndex = indexedFailures[result.Index].Index;
190-
191-
// In case the fallback function failed, we want to add the original failure to the results
192-
var unifiedResult = !isSuccessFunc(result.Item) ? indexedFailures[result.Index] : result.Item.WithIndex(originalIndex);
193-
fixedFallbackResults.Add(unifiedResult);
184+
fixedFallbackResults.Add(result.Item.WithIndex(originalIndex));
194185
}
195186

196187
// Merge original successful results with fallback results

‎Public/Src/Cache/ContentStore/InterfacesTest/Utils/WorkflowsTests.cs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT License.
33

44
using System.Collections.Generic;
5-
using System.Diagnostics;
65
using System.Linq;
76
using System.Threading.Tasks;
87
using BuildXL.Cache.ContentStore.Hashing;
@@ -57,7 +56,7 @@ public void TestRunWithThreeFallbacks()
5756
p.Index));
5857
}
5958
return Task.FromResult(new Indexed<PlaceFileResult>(
60-
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR_0"),
59+
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR"),
6160
p.Index));
6261
}));
6362
},
@@ -80,7 +79,7 @@ public void TestRunWithThreeFallbacks()
8079
p.Index));
8180
}
8281
return Task.FromResult(new Indexed<PlaceFileResult>(
83-
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR_1"),
82+
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR"),
8483
p.Index));
8584
}));
8685
},
@@ -95,7 +94,7 @@ public void TestRunWithThreeFallbacks()
9594
p =>
9695
{
9796
return Task.FromResult(new Indexed<PlaceFileResult>(
98-
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR_2"),
97+
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR"),
9998
p.Index));
10099
}));
101100
},
@@ -117,7 +116,7 @@ public void TestRunWithThreeFallbacks()
117116
p.Index));
118117
}
119118
return Task.FromResult(new Indexed<PlaceFileResult>(
120-
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR_3"),
119+
new PlaceFileResult(PlaceFileResult.ResultCode.Error, "ERROR"),
121120
p.Index));
122121
}));
123122
},
@@ -131,7 +130,6 @@ public void TestRunWithThreeFallbacks()
131130
result.ToList().ForEach(p => {
132131
if (p.Result.Index == 0) {
133132
Assert.Equal(p.Result.Item.Code, PlaceFileResult.ResultCode.Error);
134-
Assert.Equal(p.Result.Item.ErrorMessage, "ERROR_0");
135133
} else {
136134
Assert.Equal(p.Result.Item.Code, PlaceFileResult.ResultCode.PlacedWithCopy);
137135
}

0 commit comments

Comments
 (0)