Skip to content

Move hardcoded rowlimit out of the storage implementation #5899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rbev merged 5 commits into from
Sep 18, 2026
Merged

Move hardcoded rowlimit out of the storage implementation #5899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6629bc
Enable paging on group query in store, move hardcoded 200 limit into …
rbev Sep 16, 2026
8036153
Correctly include page size for group query in EF
rbev Sep 16, 2026
d9fe22b
Share the paging info between all tests
rbev Sep 16, 2026
b1efad1
Add comment about the deliberate choice on failure groups
rbev Sep 17, 2026
ecb4762
Fix up bad rebase
rbev Sep 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation;

public class GroupsDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IGroupsDataStore
{
public Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, CancellationToken cancellationToken = default) =>
public Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, PagingInfo pagingInfo, CancellationToken cancellationToken = default) =>
ExecuteWithDbContext<IList<FailureGroupView>>(async (dbContext, token) =>
{
var groups = ByClassifier(dbContext, classifier);
Expand All @@ -22,21 +22,28 @@ public Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string clas
groups = groups.Where(group => group.Title == classifierFilter);
}

var views = await GetGroupViews(groups, WithStatus(dbContext, FailedMessageStatus.Unresolved), token);
var views = await GetGroupViews(
groups,
WithStatus(dbContext, FailedMessageStatus.Unresolved),
pagingInfo,
token);

await AttachComments(dbContext, views, token);

return views;
}, cancellationToken);

public Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default) =>
public Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, PagingInfo pagingInfo, CancellationToken cancellationToken = default) =>
ExecuteWithDbContext(async (dbContext, token) =>
{
var groups = ByClassifier(dbContext, classifier);
var messages = WithStatus(dbContext, FailedMessageStatus.Archived);

var views = await GetGroupViews(groups, messages, pagingInfo, token);

var views = await GetGroupViews(groups, WithStatus(dbContext, FailedMessageStatus.Archived), token);
var totalCount = await groups.LongCountAsync(token);

return new QueryResult<IList<FailureGroupView>>(views, views.ToQueryStatsInfo("groups", views.Count));
return new QueryResult<IList<FailureGroupView>>(views, views.ToQueryStatsInfo("groups", totalCount));
}, cancellationToken);

public Task<QueryResult<FailureGroupView>> GetUnresolvedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default) =>
Expand Down Expand Up @@ -128,14 +135,15 @@ static async Task AttachComments(ServiceControlDbContext dbContext, IList<Failur
}

// One aggregate statement: Count/First/Last come from the joined message rows and the Title
// lookup per output group is bounded by FailureGroupQueries.MaxGroups. See
// lookup per output group is bounded by page size. See
// FailureGroupQueries.AggregateGroups for the provider index shapes this relies on — on
// PostgreSQL the classifier and messages indexes must carry the join column or the whole
// aggregate degrades to sequential scans of both large tables.
static async Task<List<FailureGroupView>> GetGroupViews(IQueryable<FailedMessageGroupEntity> groups, IQueryable<FailedMessageEntity> messages, CancellationToken cancellationToken) =>
static async Task<List<FailureGroupView>> GetGroupViews(IQueryable<FailedMessageGroupEntity> groups, IQueryable<FailedMessageEntity> messages, PagingInfo pagingInfo, CancellationToken cancellationToken) =>
await groups
.AggregateGroups(messages)
.OrderByDescending(summary => summary.Last)
.Take(FailureGroupQueries.MaxGroups)
.Skip(pagingInfo.Offset)
.Take(pagingInfo.PageSize)
.ToListAsync(cancellationToken);
}
2 changes: 0 additions & 2 deletions src/ServiceControl.Persistence.EFCore/Infrastructure/FailureGroupQueries.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ namespace ServiceControl.Persistence.EFCore.Infrastructure;

static class FailureGroupQueries
{
public const int MaxGroups = 200;

/// <summary>
/// The group aggregate: membership rows joined to their message, grouped by (GroupId, Type),
/// with Count/First/Last per group.
Expand Down
13 changes: 7 additions & 6 deletions src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace ServiceControl.Persistence.RavenDB.Recoverability

class GroupsDataStore(IRavenSessionProvider sessionProvider) : IGroupsDataStore
{
public async Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string classifierFilter, CancellationToken cancellationToken = default)
public async Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string classifierFilter, PagingInfo pagingInfo, CancellationToken cancellationToken = default)
{
using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken);
var query = Queryable.Where(session.Query<FailureGroupView, FailureGroupsViewIndex>(), v => v.Type == classifier);
Expand All @@ -24,8 +24,9 @@ public async Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(strin
query = query.Where(v => v.Title == classifierFilter);
}

var groups = await query.OrderByDescending(x => x.Last)
.Take(200)
var groups = await query
.OrderByDescending(view => view.Last)
.Paging(pagingInfo)
.ToListAsync(cancellationToken);

var commentIds = groups.Select(x => MakeId(x.Id)).ToArray();
Expand All @@ -40,7 +41,7 @@ public async Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(strin
return groups;
}

public async Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default)
public async Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, PagingInfo pagingInfo, CancellationToken cancellationToken = default)
{
using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken);
var groups = session
Expand All @@ -49,8 +50,8 @@ public async Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClass
.Where(v => v.Type == classifier);

var results = await groups
.OrderByDescending(x => x.Last)
.Take(200) // only show 200 groups
.OrderByDescending(view => view.Last)
.Paging(pagingInfo)
.ToListAsync(cancellationToken);

return new QueryResult<IList<FailureGroupView>>(results, stats.ToQueryStatsInfo());
Comment thread
rbev marked this conversation as resolved.
Expand Down
3 changes: 2 additions & 1 deletion src/ServiceControl.Persistence.Tests/Recoverability/ArchiveCancellationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace ServiceControl.Persistence.Tests.Recoverability;
using NUnit.Framework;
using ServiceControl.Infrastructure.DomainEvents;
using ServiceControl.MessageFailures;
using ServiceControl.Persistence.Infrastructure;
using ServiceControl.Recoverability;

[TestFixture]
Expand Down Expand Up @@ -59,7 +60,7 @@ async Task AssertNoUnresolvedMessagesIn(FailedMessage.FailureGroup group)
{
await CompleteDatabaseOperation();

var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200));

Assert.That(
groups.Select(view => view.Id),
Expand Down
16 changes: 9 additions & 7 deletions src/ServiceControl.Persistence.Tests/Recoverability/ArchivedGroupVersionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace ServiceControl.Persistence.Tests;
using System.Threading.Tasks;
using NUnit.Framework;
using ServiceControl.MessageFailures;
using ServiceControl.Persistence.Infrastructure;

[TestFixture]
class ArchivedGroupVersionTests : PersistenceTestBase
Expand All @@ -14,6 +15,7 @@ class ArchivedGroupVersionTests : PersistenceTestBase
static readonly DateTime Oldest = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc);
static readonly DateTime Middle = new(2026, 8, 1, 13, 0, 0, DateTimeKind.Utc);
static readonly DateTime Newest = new(2026, 8, 1, 17, 0, 0, DateTimeKind.Utc);
static readonly PagingInfo pagingInfo = new PagingInfo(page: 1, pageSize: 200);

[Test]
public async Task Version_changes_when_group_counts_move_but_the_total_and_the_span_hold()
Expand All @@ -28,7 +30,7 @@ public async Task Version_changes_when_group_counts_move_but_the_total_and_the_s
await Insert(oldest, middle, newest);
await Archive(oldest, middle, newest);

var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

// The archived set keeps two groups, three messages, and the same earliest and latest failure.
// All that moves is how the three are split between the groups, from two and one to one and two.
Expand All @@ -38,7 +40,7 @@ public async Task Version_changes_when_group_counts_move_but_the_total_and_the_s
_ = await FailedMessageLifecycleStore.UnArchiveMessages([middle.UniqueMessageIdString]);
await CompleteDatabaseOperation();

var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

using (Assert.EnterMultipleScope())
{
Expand All @@ -63,13 +65,13 @@ public async Task Version_changes_when_a_group_gains_a_message()
await Insert(first);
await Archive(first);

var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

var second = InGroup(group, Newest);
await Insert(second);
await Archive(second);

var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version,
"the archived group gained a message, so its validator cannot stay put");
Expand All @@ -84,8 +86,8 @@ public async Task Version_is_stable_while_nothing_changes()
await Insert(failure);
await Archive(failure);

var first = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var second = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var first = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);
var second = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version,
"nothing changed, so the validator has to stay put or conditional GET never pays off");
Expand All @@ -94,7 +96,7 @@ public async Task Version_is_stable_while_nothing_changes()
[Test]
public async Task A_classifier_with_nothing_archived_still_reports_a_version()
{
var result = await GroupsStore.GetArchivedGroupsByClassifier(Classifier);
var result = await GroupsStore.GetArchivedGroupsByClassifier(Classifier, pagingInfo);

using (Assert.EnterMultipleScope())
{
Expand Down
92 changes: 84 additions & 8 deletions src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ await Insert(
InGroup(group, failedAt: Noon),
InGroup(group, failedAt: Noon.AddHours(2)));

var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null)).Single();
var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200))).Single();

using (Assert.EnterMultipleScope())
{
Expand All @@ -46,7 +46,7 @@ public async Task Returns_only_the_groups_of_the_requested_classifier()

await Insert(InGroup(requested), InGroup(other));

var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200));

Assert.That(groups.Select(group => group.Id), Is.EqualTo(new[] { requested.Id }));
}
Expand All @@ -59,7 +59,7 @@ public async Task Narrows_the_groups_to_the_classifier_filter()

await Insert(InGroup(matching), InGroup(other));

var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, "OrderPlaced");
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, "OrderPlaced", new PagingInfo(page: 1, pageSize: 200));

Assert.That(groups.Select(group => group.Id), Is.EqualTo(new[] { matching.Id }));
}
Expand All @@ -74,7 +74,7 @@ await Insert(
InGroup(group).ToFailedMessage(FailedMessageStatus.Archived),
InGroup(group).ToFailedMessage(FailedMessageStatus.Resolved));

var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null)).Single();
var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200))).Single();

Assert.That(view.Count, Is.EqualTo(1));
}
Expand All @@ -88,7 +88,7 @@ await Insert(
InGroup(group).ToFailedMessage(),
InGroup(group).ToFailedMessage(FailedMessageStatus.Archived));

var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Results.Single();
var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier, new PagingInfo(page: 1, pageSize: 200))).Results.Single();

using (Assert.EnterMultipleScope())
{
Expand All @@ -109,11 +109,87 @@ await Insert(
InGroup(newest, failedAt: Noon.AddHours(4)),
InGroup(middle, failedAt: Noon.AddHours(2)));

var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200));

Assert.That(groups.Select(group => group.Title), Is.EqualTo(new[] { "Newest", "Middle", "Oldest" }));
}

[Test]
public async Task Can_page_unresolved_groups()
{
var oldest = NewGroup("Oldest");
var older = NewGroup("Older");
var middle = NewGroup("Middle");
var newer = NewGroup("Newer");
var newest = NewGroup("Newest");

await Insert(
InGroup(oldest, failedAt: Noon),
InGroup(older, failedAt: Noon.AddHours(1)),
InGroup(middle, failedAt: Noon.AddHours(2)),
InGroup(newer, failedAt: Noon.AddHours(3)),
InGroup(newest, failedAt: Noon.AddHours(4)));

var firstPage = await GroupsStore.GetUnresolvedGroupsByClassifier(
Classifier,
null,
new PagingInfo(page: 1, pageSize: 2));

var secondPage = await GroupsStore.GetUnresolvedGroupsByClassifier(
Classifier,
null,
new PagingInfo(page: 2, pageSize: 2));

using (Assert.EnterMultipleScope())
{
Assert.That(firstPage, Has.Count.EqualTo(2));
Assert.That(secondPage, Has.Count.EqualTo(2));
Assert.That(firstPage, Is.Ordered.By(nameof(FailureGroupView.Last)).Descending);
Assert.That(
secondPage.Select(group => group.Id).Intersect(firstPage.Select(group => group.Id)),
Is.Empty);
}
}

[Test]
public async Task Can_page_archived_groups()
{
var oldest = NewGroup("Oldest");
var older = NewGroup("Older");
var middle = NewGroup("Middle");
var newer = NewGroup("Newer");
var newest = NewGroup("Newest");

await Insert(
InGroup(oldest, failedAt: Noon).ToFailedMessage(FailedMessageStatus.Archived),
InGroup(older, failedAt: Noon.AddHours(1)).ToFailedMessage(FailedMessageStatus.Archived),
InGroup(middle, failedAt: Noon.AddHours(2)).ToFailedMessage(FailedMessageStatus.Archived),
InGroup(newer, failedAt: Noon.AddHours(3)).ToFailedMessage(FailedMessageStatus.Archived),
InGroup(newest, failedAt: Noon.AddHours(4)).ToFailedMessage(FailedMessageStatus.Archived));

var firstPage = await GroupsStore.GetArchivedGroupsByClassifier(
Classifier,
new PagingInfo(page: 1, pageSize: 2));

var secondPage = await GroupsStore.GetArchivedGroupsByClassifier(
Classifier,
new PagingInfo(page: 2, pageSize: 2));

using (Assert.EnterMultipleScope())
{
Assert.That(firstPage.Results, Has.Count.EqualTo(2));
Assert.That(secondPage.Results, Has.Count.EqualTo(2));
Assert.That(firstPage.Results, Is.Ordered.By(nameof(FailureGroupView.Last)).Descending);
Assert.That(
secondPage.Results.Select(group => group.Id).Intersect(firstPage.Results.Select(group => group.Id)),
Is.Empty);
Assert.That(firstPage.QueryStats.TotalCount, Is.EqualTo(5),
"the total count should reflect all matching groups, not just the page");
Assert.That(secondPage.QueryStats.TotalCount, Is.EqualTo(5),
"the total count should reflect all matching groups, not just the page");
}
}

[Test]
public async Task Returns_a_single_group_by_id()
{
Expand Down Expand Up @@ -295,14 +371,14 @@ public async Task Leaves_archived_groups_without_their_comment()
await Insert(InGroup(group).ToFailedMessage(FailedMessageStatus.Archived));
await EditComment(group.Id, "Only shown on the open group");

var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Results.Single();
var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier, new PagingInfo(page: 1, pageSize: 200))).Results.Single();

Assert.That(view.Comment, Is.Null);
}

async Task<string> CommentFor(FailedMessage.FailureGroup group)
{
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null, new PagingInfo(page: 1, pageSize: 200));

return groups.Single(view => view.Id == group.Id).Comment;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ServiceControl.Persistence/IGroupsDataStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ namespace ServiceControl.Persistence

public interface IGroupsDataStore
{
Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, CancellationToken cancellationToken = default);
Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default);
Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, PagingInfo pagingInfo, CancellationToken cancellationToken = default);
Task<QueryResult<IList<FailureGroupView>>> GetArchivedGroupsByClassifier(string classifier, PagingInfo pagingInfo, CancellationToken cancellationToken = default);

Task<QueryResult<FailureGroupView>> GetUnresolvedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default);
Task<QueryResult<FailureGroupView>> GetArchivedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default);
Expand Down
Loading