Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public static IUmbracoBuilder AddExamineSearchProvider(this IUmbracoBuilder buil

// NOTE: this notification handler should ONLY be active when zero downtime reindexing is in effect
builder.AddNotificationHandler<IndexRebuildStartingNotification, ZeroDowntimeRebuildNotificationHandler>();
builder.AddNotificationHandler<IndexRebuildCompletedNotification, ZeroDowntimeRebuildNotificationHandler>();
builder.AddNotificationAsyncHandler<IndexRebuildCompletedNotification, ZeroDowntimeRebuildNotificationHandler>();
}
else
{
Expand All @@ -59,6 +59,8 @@ public static IUmbracoBuilder AddExamineSearchProvider(this IUmbracoBuilder buil
builder.Services.AddSingleton<IActiveIndexManager, NoopActiveIndexManager>();
}

builder.Services.AddSingleton<IIndexCommitMonitor, IndexCommitMonitor>();

// This is needed, due to locking of indexes on Azure, to read more on this issue go here: https://github.com/umbraco/Umbraco-CMS/pull/15571
builder.Services.AddSingleton<UmbracoTempEnvFileSystemDirectoryFactory>();
builder.Services.RemoveAll<SyncedFileSystemDirectoryFactory>();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Examine;
using Examine.Lucene.Providers;
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Search.Core.Notifications;
Expand All @@ -10,35 +9,48 @@ namespace Umbraco.Cms.Search.Provider.Examine.NotificationHandlers;
// NOTE: This notification handler is only active when zero downtime reindexing is in effect
internal sealed class ZeroDowntimeRebuildNotificationHandler :
INotificationHandler<IndexRebuildStartingNotification>,
INotificationHandler<IndexRebuildCompletedNotification>
INotificationAsyncHandler<IndexRebuildCompletedNotification>
{
private static readonly TimeSpan CommitTimeout = TimeSpan.FromSeconds(30);

private readonly IActiveIndexManager _activeIndexManager;
private readonly IExamineManager _examineManager;
private readonly IIndexCommitMonitor _indexCommitMonitor;
private readonly ILogger<ZeroDowntimeRebuildNotificationHandler> _logger;

public ZeroDowntimeRebuildNotificationHandler(
IActiveIndexManager activeIndexManager,
IExamineManager examineManager,
IIndexCommitMonitor indexCommitMonitor,
ILogger<ZeroDowntimeRebuildNotificationHandler> logger)
{
_activeIndexManager = activeIndexManager;
_examineManager = examineManager;
_indexCommitMonitor = indexCommitMonitor;
_logger = logger;
}

public void Handle(IndexRebuildStartingNotification notification)
=> _activeIndexManager.StartRebuilding(notification.IndexAlias);

public void Handle(IndexRebuildCompletedNotification notification)
public async Task HandleAsync(IndexRebuildCompletedNotification notification, CancellationToken cancellationToken)
{
var shadowIndexName = _activeIndexManager.ResolveShadowIndexName(notification.IndexAlias);

// Examine's LuceneIndex.IndexItems() commits asynchronously. We must wait for the
// commit to complete before checking document count, otherwise we'll see 0 documents
// and incorrectly cancel the swap.
WaitForShadowCommit(shadowIndexName);
var committed = await _indexCommitMonitor.WaitForCommitAsync(shadowIndexName, cancellationToken);
if (cancellationToken.IsCancellationRequested)
{
_logger.LogInformation("Cancellation requested before completion of shadow index swap for {ShadowIndex}.", shadowIndexName);
return;
}

if (committed is false)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when this occurs? is the expectation that maybe the shadow index will still be healthy? Or that the committing/monitor has failed and this needs to exit?

{
_logger.LogWarning("Timed out waiting for shadow index {ShadowIndex} to commit after rebuild", shadowIndexName);
}

if (IsShadowIndexHealthy(shadowIndexName))
{
Expand All @@ -55,48 +67,6 @@ public void Handle(IndexRebuildCompletedNotification notification)
}
}

private void WaitForShadowCommit(string physicalIndexName)
{
if (_examineManager.TryGetIndex(physicalIndexName, out IIndex? index) is false || index is not LuceneIndex luceneIndex)
{
return;
}

if (index is IIndexStats stats && stats.GetDocumentCount() > 0)
{
return;
}

var committed = false;
void OnCommitted(object? sender, EventArgs e) => committed = true;
luceneIndex.IndexCommitted += OnCommitted;

try
{
// Re-check after subscribing to avoid a race where the commit happened
// between the initial check and subscribing to the event.
if (index is IIndexStats statsAfterSubscribe && statsAfterSubscribe.GetDocumentCount() > 0)
{
return;
}

var stopwatch = System.Diagnostics.Stopwatch.StartNew();
while (!committed && stopwatch.Elapsed < CommitTimeout)
{
Thread.Sleep(100);
}

if (!committed)
{
_logger.LogWarning("Timed out waiting for shadow index {ShadowIndex} to commit after rebuild", physicalIndexName);
}
}
finally
{
luceneIndex.IndexCommitted -= OnCommitted;
}
}

private void ClearShadowIndex(string indexAlias)
{
var shadowIndexName = _activeIndexManager.ResolveShadowIndexName(indexAlias);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace Umbraco.Cms.Search.Provider.Examine.Services;

public interface IIndexCommitMonitor
{
Task<bool> WaitForCommitAsync(string indexAlias, CancellationToken cancellationToken);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using Examine;
using Examine.Lucene.Providers;

namespace Umbraco.Cms.Search.Provider.Examine.Services;

internal sealed class IndexCommitMonitor : IIndexCommitMonitor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, this i sort of what is proposed here umbraco/Umbraco-CMS#16546 which links on to here Shazwazza/Examine#372 (comment)

Using the event can be problematic, but depends on the use case and outcome. If this is to just wait for a single commit than its probably ok but if its to wait for a rebuild, then the strategy mentioned in the links would be far better where a final special document/record is indexed after the populators run to indicate that rebuild is complete.

{
private static readonly TimeSpan CommitTimeout = TimeSpan.FromSeconds(30);

private readonly IExamineManager _examineManager;

public IndexCommitMonitor(IExamineManager examineManager)
=> _examineManager = examineManager;

public async Task<bool> WaitForCommitAsync(string indexAlias, CancellationToken cancellationToken)
{
if (_examineManager.TryGetIndex(indexAlias, out IIndex? index) is false || index is not LuceneIndex luceneIndex)
{
return false;
}

if (index is IIndexStats stats && stats.GetDocumentCount() > 0)
{
return false;
}

var committed = false;
EventHandler onCommitted = (_, _) => committed = true;

try
{
luceneIndex.IndexCommitted += onCommitted;

// Re-check after subscribing to avoid a race where the commit happened
// between the initial check and subscribing to the event.
if (index is IIndexStats statsAfterSubscribe && statsAfterSubscribe.GetDocumentCount() > 0)
{
return true;
}

var stopwatch = System.Diagnostics.Stopwatch.StartNew();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never gets stopped?

while (!committed && stopwatch.Elapsed < CommitTimeout)
{
await Task.Delay(CommitTimeout);
}

return committed;
}
finally
{
luceneIndex.IndexCommitted -= onCommitted;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public static IServiceCollection AddExamineSearchProviderServicesForTest<TIndex,
// Override to use ActiveIndexManager for zero-downtime reindexing in integration tests
services.AddSingleton<IActiveIndexManager, ActiveIndexManager>();

services.AddSingleton<IIndexCommitMonitor, IndexCommitMonitor>();

return services;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static IUmbracoBuilder AddExamineSearchProviderForTest<TIndex, TDirectory
builder.AddNotificationHandler<MemberSavedNotification, MemberSavedDistributedCacheNotificationHandler>();
builder.AddNotificationHandler<MemberDeletedNotification, MemberDeletedDistributedCacheNotificationHandler>();
builder.AddNotificationHandler<IndexRebuildStartingNotification, ZeroDowntimeRebuildNotificationHandler>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expectation that the ZeroDowntimeRebuildNotificationHandler is only for Lucene indexes or for any Examine based index?

builder.AddNotificationHandler<IndexRebuildCompletedNotification, ZeroDowntimeRebuildNotificationHandler>();
builder.AddNotificationAsyncHandler<IndexRebuildCompletedNotification, ZeroDowntimeRebuildNotificationHandler>();

return builder;
}
Expand Down