paulirwin commented on code in PR #1182:
URL: https://github.com/apache/lucenenet/pull/1182#discussion_r2551918542
##########
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:
There is another problem with the current approach of having two
constructors: the constructors are ambiguous if you try to pass in a
`HttpReplicator` without casting it to either interface first:
<img width="1118" height="314" alt="screenshot-2025-11-21-at-20-19-15"
src="https://github.com/user-attachments/assets/f154c390-fd9b-4b99-aa37-0040d0768780";
/>
Our options include:
1. Accept that you have to cast this before passing it in. I think this is a
poor design choice for other reasons mentioned above, but also having to impose
this on users is not ideal. Note that it's also technically a breaking change
as-is, since we'd require people to cast HttpReplicator.
2. Remove the IAsyncReplicator interface and move its async methods into
IReplicator (so that it would have both async and sync methods), making any
custom implementations either implement the async methods or throw
NotImplementedException. This would be a breaking change because you'd make
custom implementations add these async methods.
3. A slight alternative to option 2 but with the same effect: make
IAsyncReplicator inherit from IReplicator, and have ReplicationClient take only
an IAsyncReplicator (which would now provide both methods). This is a breaking
change since users of ReplicationClient with custom IReplicators would have to
make their custom replicator implement IAsyncReplicator as well.
4. Remove synchronous replication support and lean in to async only. This
would of course be a breaking change, but it would simplify the API.
5. Split out an AsyncReplicationClient as discussed above.
AsyncReplicationClient would take an IAsyncReplicator; ReplicationClient would
take a Replicator. This is the only non-breaking way to do this.
@NightOwl888 thoughts? I could go for any of these except for option 1. I
think if we're going to have any breaking changes we might as well do option 4
and just remove synchronous replication support to keep it simple, but I'm
inclined to go for option 5 and just not have the breaking change.
--
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]
