Shazwazza commented on code in PR #1182:
URL: https://github.com/apache/lucenenet/pull/1182#discussion_r2738028203
##########
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:
We can't do this:
> Remove synchronous replication support and lean in to async only. This
would of course be a breaking change, but it would simplify the API.
synchronous replication is still done and used by the
IndexReplicationHandler and IndexAndTaxonomyReplicationHandler which just
replicate between directories/indexes (locally and synchronously)
I mean, i guess you can wrap the sync over async with Task.FromResult but
this also adds some overhead - unless we use ValueTask which might be a thing
to do anyways. This will be quite a big breaking change though.
Why not do this:
* Keep IAsyncReplicator (maybe use ValueTask if it fits the design)
* Make IAsyncReplicator inherit from IReplicator
* Keep only one ctor that accepts IReplicator - then type check to see if it
implements IAsyncReplicator and store it in a nullable field
* Add the async methods to the ReplicationClient as needed and within these
methods, check if the IAsyncReplicator is null or not, if its not null, use it
and return values. If it is null, get the result from the non-async IReplicator
and return the value with Task.FromResult. Similarly, for the new async
methods, perform this check but if the IAsyncReplicator field is null, either:
throw a NotSupportedException, or return GetAwaiter().GetResult()
This shouldn't introduce breaking changes and will support the async
requirements since the user will now what their implementation is i.e.
UpdateNow vs UpdateNowAsync
--
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]
