NightOwl888 commented on PR #1170: URL: https://github.com/apache/lucenenet/pull/1170#issuecomment-3238093592
> @NightOwl888 > > just a quick update — I had to **force push** this branch. > > Reason: in last commit I guess you removed the `Perform()` method from `ReplicationServlet` entirely. I’ve corrected that by **commenting it out instead of deleting** so the history/context remains clear. > > Other changes I pushed: > > * Added an **XML summary** for the new `PerformAsync()` method in the extension. > * Cleaned up some **duplicate commented code**. > * Added new **async read/write tests** (adapted from J2N) — and all test cases are passing. ✅ > > Please let me know if you notice anything odd or if you’d prefer a different approach. Thanks. Removing `Perform()` wasn't intentional. And thanks for adding the tests. Although, I realized that by switching our tests to using async only, we are no longer testing the synchronous methods. I am looking into how to set it up without too much code duplication. I think we pretty much have to duplicate everything in `ReplicationServlet`. The `NewHttpServer()` method overloads can get a bool `async` parameter. Then comes the tricky bit. I think the `HttpReplicatorTest` class could be converted into a `HttpReplicationServerTestCase` that is abstract. Then an abstract bool property could be added named `UseAsyncServer`, which would be passed into `NewHttpServer()`. Then we could just make 2 new classes, `AsyncHttpReplicatorTest` and `HttpReplicatorTest` that subclass `HttpReplicationServerTestCase`, each setting the bool value appropriately. I am about to turn in for the night, but would be happy to work on it tomorrow if you don't get the chance. -- 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: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
