Skip to content
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.

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

TEST OF REVIEW #11

Merged
merged 6 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
5 changes: 0 additions & 5 deletions src/DotJEM.Json.Index2.Management/IJsonIndexManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,6 @@ public async Task ResetIndexAsync()
{
await jsonDocumentSource.StopAsync().ConfigureAwait(false);
index.Storage.Delete();
//TODO: Force a commit after delete to see if that helps?
using (var lease = index.WriterManager.Lease())
{
lease.Value.Commit();
}
await jsonDocumentSource.ResetAsync().ConfigureAwait(false);
await jsonDocumentSource.StartAsync().ConfigureAwait(false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using DotJEM.Json.Index2.Snapshots;
using DotJEM.ObservableExtensions.InfoStreams;
using DotJEM.Web.Scheduler;
using Lucene.Net.Search;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

Expand Down Expand Up @@ -41,6 +42,8 @@ public class JsonIndexSnapshotManager : IJsonIndexSnapshotManager
{
private readonly IJsonIndex index;
private readonly ISnapshotStrategy strategy;
//TODO: This binds us to a concrete idea (even though this is an interface).
// instead it would be preferred if we could provide an abstraction for what is needed.
Comment on lines +45 to +46
Copy link

Choose a reason for hiding this comment

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

Address the design concern by refactoring the code to depend on an abstraction.

The TODO comment correctly identifies a design issue where the JsonIndexSnapshotManager class is tightly coupled to a concrete implementation of the snapshot strategy. This makes the code less flexible and harder to extend or modify.

Consider refactoring the code to depend on an interface instead of a concrete implementation. This aligns with the Dependency Inversion Principle (DIP) of SOLID and would make the code more flexible and easier to test.

Here's a high-level overview of the refactoring steps:

  1. Define an interface that encapsulates the behavior of the snapshot strategy. For example:
public interface ISnapshotStrategy
{
    IInfoStream InfoStream { get; }
    ISnapshotStorage Storage { get; }
    void CleanOldSnapshots();
    // Other methods as needed
}
  1. Update the JsonIndexSnapshotManager constructor to depend on the ISnapshotStrategy interface instead of a concrete implementation:
-public JsonIndexSnapshotManager(IJsonIndex index, ISnapshotStrategy snapshotStrategy, IWebTaskScheduler scheduler, string schedule)
+public JsonIndexSnapshotManager(IJsonIndex index, ISnapshotStrategy snapshotStrategy, IWebTaskScheduler scheduler, string schedule)
{
    this.index = index;
    this.scheduler = scheduler;
    this.schedule = schedule;
-   this.strategy = snapshotStrategy;
+   this.strategy = snapshotStrategy;
    this.strategy.InfoStream.Subscribe(infoStream);
}
  1. Update the code that creates an instance of JsonIndexSnapshotManager to pass in an implementation of ISnapshotStrategy.

This refactoring would decouple the JsonIndexSnapshotManager from a specific snapshot strategy implementation, making it more flexible and easier to extend or modify in the future.

private readonly IWebTaskScheduler scheduler;
private readonly IInfoStream<JsonIndexSnapshotManager> infoStream = new InfoStream<JsonIndexSnapshotManager>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using DotJEM.Json.Index2.Documents;
using DotJEM.Json.Index2.Documents.Info;
using DotJEM.Json.Index2.IO;
using DotJEM.Json.Index2.Leases;
using DotJEM.ObservableExtensions.InfoStreams;
using Lucene.Net.Index;
using Newtonsoft.Json.Linq;
Expand Down
1 change: 1 addition & 0 deletions src/DotJEM.Json.Index2.Snapshots/IndexSnapshotHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using DotJEM.Json.Index2.IO;
using DotJEM.Json.Index2.Leases;
using DotJEM.Json.Index2.Snapshots.Streams;
using Lucene.Net.Index;
using Lucene.Net.Store;
Expand Down
1 change: 1 addition & 0 deletions src/DotJEM.Json.Index2/IO/JsonIndexWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Linq;
using DotJEM.Json.Index2.Documents;
using DotJEM.Json.Index2.Leases;
using DotJEM.Json.Index2.Util;
using Lucene.Net.Documents;
using Lucene.Net.Index;
Expand Down
130 changes: 15 additions & 115 deletions src/DotJEM.Json.Index2/IO/JsonIndexWriterManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,24 @@
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using DotJEM.Json.Index2.Configuration;
using DotJEM.Json.Index2.Leases;
using DotJEM.Json.Index2.Util;
using Lucene.Net.Analysis;
using Lucene.Net.Index;
using Lucene.Net.Search;

namespace DotJEM.Json.Index2.IO;

public interface IIndexWriterManager : IDisposable
{
event EventHandler<EventArgs> OnClose;

ILease<IndexWriter> Lease();
void Close();
}

public interface ILease<out T> : IDisposable
{
T Value { get; }
bool IsExpired { get; }
}

public class IndexWriterManager : Disposable, IIndexWriterManager
{
Expand All @@ -31,8 +30,9 @@ public class IndexWriterManager : Disposable, IIndexWriterManager
private readonly IJsonIndex index;
private volatile IndexWriter writer;
private readonly object writerPadLock = new();
private readonly object leasesPadLock = new();
private readonly LeaseManager<IndexWriter> leaseManager = new();

//TODO: With leases, this should not be needed.
public event EventHandler<EventArgs> OnClose;

private IndexWriter Writer
Expand All @@ -47,40 +47,13 @@ private IndexWriter Writer
if (writer != null)
return writer;

try
{
return writer = Open(index);
}
catch (Exception e)
{
Debug.WriteLine("ACTIVE LEASES: " +leases.Count);
throw;
}
return writer = Open(index);
}
}
}

private readonly List<TimeLimitedIndexWriterLease> leases = new List<TimeLimitedIndexWriterLease>();

public ILease<IndexWriter> Lease()
{
//TODO: Optimizied collection for this.
TimeLimitedIndexWriterLease lease = new(this, OnReturned);
lock (leasesPadLock)
{
leases.Add(lease);
}
return lease;
}

private void OnReturned(TimeLimitedIndexWriterLease lease)
{
lock (leasesPadLock)
{
leases.Remove(lease);
}
}

public ILease<IndexWriter> Lease() => leaseManager.Create(Writer, TimeSpan.FromSeconds(3));

public IndexWriterManager(IJsonIndex index)
{
Expand All @@ -89,7 +62,6 @@ public IndexWriterManager(IJsonIndex index)

private static IndexWriter Open(IJsonIndex index)
{
Debug.WriteLine("OPEN WRITER");
IndexWriterConfig config = new(index.Configuration.Version, index.Configuration.Analyzer);
config.RAMBufferSizeMB = DEFAULT_RAM_BUFFER_SIZE_MB;
config.OpenMode = OpenMode.CREATE_OR_APPEND;
Expand All @@ -103,28 +75,15 @@ public void Close()
if (writer == null)
return;

lock (leasesPadLock)
lock (writerPadLock)
{
TimeLimitedIndexWriterLease[] leasesCopy = leases.ToArray();

Debug.WriteLine("ACTIVE LEASES: " + leasesCopy.Length);
leases.Clear();
foreach (TimeLimitedIndexWriterLease lease in leasesCopy)
{
if (!lease.IsExpired)
lease.Wait();
lease.Dispose();
}

lock (writerPadLock)
{
if (writer == null)
return;
leaseManager.RecallAll();
if (writer == null)
return;

writer.Dispose();
writer = null;
RaiseOnClose();
}
writer.Dispose();
writer = null;
RaiseOnClose();
}
}

Expand All @@ -140,63 +99,4 @@ protected virtual void RaiseOnClose()
{
OnClose?.Invoke(this, EventArgs.Empty);
}

private class TimeLimitedIndexWriterLease : Disposable, ILease<IndexWriter>
{
private readonly DateTime leaseTime = DateTime.Now;
private readonly Action<TimeLimitedIndexWriterLease> onReturned;
private readonly IndexWriterManager manager;
public AutoResetEvent Handle { get; } = new(false);

public IndexWriter Value
{
get
{
if (IsDisposed)
{
throw new ObjectDisposedException("Index writer lease has been returned or is expired.");
}

if (IsExpired)
{
throw new LeaseExpiredException("Index writer lease has been returned or is expired.");
}
return manager.Writer;
}
}

public bool IsExpired => (DateTime.Now - leaseTime > TimeSpan.FromSeconds(5)) || IsDisposed;

public TimeLimitedIndexWriterLease(IndexWriterManager manager, Action<TimeLimitedIndexWriterLease> onReturned)
{
this.onReturned = onReturned;
this.manager = manager;
}

protected override void Dispose(bool disposing)
{
Handle.Set();
onReturned(this);
}

public void Wait()
{
if (IsExpired)
return;

Handle.WaitOne(TimeSpan.FromSeconds(6) - (DateTime.Now - leaseTime));
}
}

}

public class LeaseExpiredException : Exception
{
public LeaseExpiredException(string message) : base(message)
{
}

public LeaseExpiredException(string message, Exception innerException) : base(message, innerException)
{
}
}
18 changes: 18 additions & 0 deletions src/DotJEM.Json.Index2/Leases/Lease.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
using System;

namespace DotJEM.Json.Index2.Leases;

public interface ILease<out T> : IDisposable
{
event EventHandler<EventArgs> Terminated;

T Value { get; }
bool IsExpired { get; }
bool TryRenew();
}


public interface ILessor<out T>
{
ILease<T> Lease();
}
Loading
Loading