paulirwin commented on code in PR #1182:
URL: https://github.com/apache/lucenenet/pull/1182#discussion_r2336629792
##########
src/Lucene.Net.Replicator/Http/HttpClientBase.cs:
##########
@@ -200,6 +200,29 @@ protected virtual HttpResponseMessage ExecuteGet(string
request, params string[]
return Execute(req);
}
+ /// <summary>
+ /// Execute a GET request asynchronously with an array of parameters.
+ /// </summary>
+ protected Task<HttpResponseMessage> ExecuteGetAsync(string action,
string[] parameters, CancellationToken cancellationToken)
+ {
+ var url = BuildUrl(action, parameters);
Review Comment:
The other methods call EnsureOpen first. Please do so here.
##########
src/Lucene.Net.Replicator/IAsyncReplicator.cs:
##########
@@ -0,0 +1,35 @@
+using System.IO;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Lucene.Net.Replicator
+{
+ /// <summary>
+ /// Async version of <see cref="IReplicator"/> for non-blocking
replication operations.
+ /// </summary>
+ public interface IAsyncReplicator
+ {
+ /// <summary>
+ /// Check whether the given version is up-to-date and returns a
+ /// <see cref="SessionToken"/> which can be used for fetching the
revision files.
+ /// </summary>
+ /// <param name="currentVersion">Current version of the index.</param>
+ /// <param name="cancellationToken">Cancellation token.</param>
+ Task<SessionToken?> CheckForUpdateAsync(string currentVersion,
CancellationToken cancellationToken = default);
Review Comment:
Add `#nullable enable` to this file to resolve these warnings.
##########
src/Lucene.Net.Replicator/ReplicationClient.cs:
##########
@@ -261,6 +279,101 @@ private void DoUpdate()
}
}
+ /// <summary>
+ /// Performs the async update logic, mirrors DoUpdate but uses
IAsyncReplicator.
+ /// </summary>
+ private async Task DoUpdateAsync(CancellationToken cancellationToken)
+ {
+ if (asyncReplicator is null)
+ throw new InvalidOperationException("AsyncReplicator not
initialized.");
+
+ SessionToken? session = null;
Review Comment:
Please add `#nullable enable` to this file to resolve these warnings, and
fix any nullability issues that might arise as a result.
##########
src/Lucene.Net.Replicator/Http/HttpReplicator.cs:
##########
@@ -106,5 +109,95 @@ public virtual void Release(string sessionId)
// do not remove this call: as it is still validating for us!
DoAction<object>(response, () => null);
}
+
+ #region Async methods (IAsyncReplicator)
+
+ /// <summary>
+ /// Checks for updates at the remote host asynchronously.
+ /// </summary>
+ /// <param name="currentVersion">The current index version.</param>
+ /// <param name="cancellationToken">Cancellation token.</param>
+ /// <returns>
+ /// A <see cref="SessionToken"/> if updates are available; otherwise,
<c>null</c>.
+ /// </returns>
+ public async Task<SessionToken?> CheckForUpdateAsync(string
currentVersion, CancellationToken cancellationToken = default)
Review Comment:
Please add `#nullable enable` to the top of this file to resolve these
warnings, and then if that exposes other nullability issues in the file, please
fix them.
##########
src/Lucene.Net.Replicator/ReplicationClient.cs:
##########
@@ -151,6 +155,20 @@ public ReplicationClient(IReplicator replicator,
IReplicationHandler handler, IS
this.factory = factory;
}
+ /// <summary>
+ /// Constructor for async replicators.
+ /// </summary>
+ /// <param name="asyncReplicator"></param>
+ /// <param name="handler"></param>
+ /// <param name="factory"></param>
+ /// <exception cref="ArgumentNullException"></exception>
+ public ReplicationClient(IAsyncReplicator asyncReplicator,
IReplicationHandler handler, ISourceDirectoryFactory factory)
Review Comment:
So, this is a brittle design. It means that you have to know which
constructor was used in order to know which methods to call and not get an
exception at runtime. I think we should break this type apart.
Let me know your thoughts on this:
1. Refactor out a common ReplicationClientBase abstract class with any
common methods/fields
2. Make this type inherit from the base class. Leave `// LUCENENET: `
comments where methods were refactored into the base class, to aid future
porting efforts. Move the async methods into item 3 below.
3. Introduce a new AsyncReplicationClient that inherits from
ReplicationClientBase, and only has the async methods. This way you can know
that it will have an async replicator field.
Thoughts?
##########
src/Lucene.Net.Replicator/ReplicationClient.cs:
##########
@@ -468,6 +695,25 @@ public virtual void UpdateNow()
}
}
+ /// <summary>
+ /// Executes the update operation asynchronously immediately,
regardless if an update thread is running or not.
+ /// </summary>
+ public virtual async Task UpdateNowAsync(CancellationToken
cancellationToken = default)
+ {
+ EnsureOpen();
+
+ // Acquire the same update lock to prevent concurrent updates
+ updateLock.Lock();
Review Comment:
Question for @NightOwl888: is this async-safe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
