Github user AndyPook commented on the issue:

    https://github.com/apache/lucenenet/pull/209
  
    All good :) In no way is any criticism intended. Was just looking at it
    from a micro/code perspective.
    The switch is just fine as is, the case blocks are small, maybe just
    extract it out into another method to isolate it from the setup and
    exception catch stuff which is always noisy. If there's a desire to plug
    into other http frameworks then the IReplicatorReq/Res works well too
    
    Re: other frameworks... I think that, as we're dealing with http rather
    than the higher framework, that it more depends on how it's all
    bootstrapped. So owin style or Global.asax.
    I think that it would be a very similar approach for webapi2/mvc4/5 as they
    both have a notion of an owin pipeline. Though the types are different I
    think. Not sure DI is a blocker here, wiring the bits up seems straight
    forward enough.
    For non owin, "all that's needed" is someplace with the current HttpContext
    to to call the ReplicatorService if the path StartsWith urlPrefix. So, an
    IHttpHandler or from GlobalAsax.BeginRequest
    
    As you've kept the api to Replicator abstract then adding other frameworks
    ought to be ok.
    Happy to contribute if that'd help at all.
    
    On 8 August 2017 at 14:07, Shad Storhaug <notificati...@github.com> wrote:
    
    > @AndyPook <https://github.com/andypook>
    >
    > Good point on the context parameter. After taking a closer look
    > 
<https://github.com/apache/lucene-solr/blob/releases/lucene-solr/4.8.1/lucene/replicator/src/java/org/apache/lucene/replicator/http/HttpClientBase.java#L77-L78>,
    > there is a path parameter on the client. Why they didn't name it context
    > to match the server is beyond me, but it appears to be one and the same.
    > So, effectively, we can get rid of that parameter and replace it with
    > urlPrefix because they are the same thing. And because we have a prefix
    > (and indeed it is not optional), we don't need a route constraint.
    >
    > Funny how long it is taking us to work this out since they admitted on
    > that blog post that they built replicator in just 1 day. But they 
certainly
    > could have documented it better in the API docs instead of falling back on
    > a blog to serve as the documentation.
    >
    > Actually, the above code wasn't intended to be a finished product, but a
    > demonstration of how to fit the pieces together. And making the pieces fit
    > first (and tests pass) before refactoring is a good strategy. One decision
    > that I will probably end up changing is to make the parameter
    > IReadOnlyDictionary instead of IDictionary, since we don't want anyone
    > tinkering with the contents of this singleton after application startup.
    >
    > While I agree with you that the switch case statement in
    > ReplicationService seems to be screaming out for refactoring, I think
    > Jens is also right on the money on this approach. Being that there needs 
to
    > be some server-side interaction between the runtime code and HTTP 
listener,
    > it is not possible to just wrap this up into a generic service that can be
    > installed with an installer. In fact, we should build similar integration
    > packages for MVC 5, WebApi2, and possibly even web forms. And if we are
    > doing that, we should wait to see how we can create those integration
    > packages before refactoring ReplicationService (assuming refactoring it
    > is even in the cards - it may ultimately prove to be more useful to serve
    > as the generic "servlet" piece that is missing from .NET). I think what
    > Jens did with the IReplicationRequest and IReplicationResponse
    > abstractions is a fine idea that will ultimately prove useful for plugging
    > into each of these frameworks. And if ReplicationService is something the
    > user never has to deal with anyway, maybe it doesn't make sense to change
    > it as tempting as that might be. After all, if we refactor it, then we 
have
    > to also refactor the tests.
    >
    > @jeme <https://github.com/jeme>
    >
    > I am not implying you have to do all of the work to integrate with each of
    > those frameworks, just finishing AspNetCore is enough. Actually,
    > integrating with the other frameworks will be a bit more tricky because 
the
    > lack of default DI container, so I the most intuitive approach might be to
    > register the dictionary statically on those.
    >
    > I have arrived at a decision on a folder structure for the .NET wrapper
    > projects. We should put Lucene.Net.Replicator.AspNetCore into a new
    > directory src/dotnet/. This will be the place where all of the projects
    > go that we aren't specifically porting from Java, and eventually we can
    > move the Lucene.Net.ICU and lucene-cli projects and their tests there,
    > too. However, if you want to give this new project a permanent home, go
    > ahead and create that folder as part of this PR.
    >
    > This will also serve as a "contrib" folder, but to me calling a project
    > "contrib" makes it sound like it is unfinished or inferior quality. We
    > expect that these packages will be highly polished with a more intuitive
    > API than the Java ported code.
    >
    > —
    > You are receiving this because you were mentioned.
    > Reply to this email directly, view it on GitHub
    > <https://github.com/apache/lucenenet/pull/209#issuecomment-320950344>, or 
mute
    > the thread
    > 
<https://github.com/notifications/unsubscribe-auth/AAvrq_7oi_Bmir0DbB5HClGLqLo34yw9ks5sWF2PgaJpZM4OgKD6>
    > .
    >



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to