Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle or forward #11

Merged
merged 25 commits into from
Oct 3, 2016
Merged

Handle or forward #11

merged 25 commits into from
Oct 3, 2016

Conversation

ashwanthkumar
Copy link
Owner

Fixes #2

Early version of HandleOrForward based on a ForwardStrategy. Will integrate with Partitioner once #10 is merged.

cc: @brewkode

@ashwanthkumar ashwanthkumar mentioned this pull request Oct 2, 2016
4 tasks
@brewkode
Copy link
Collaborator

brewkode commented Oct 2, 2016

Making some changes towards better naming. Calling ForwardStrategy as RoutingStrategy. handleOrForward could be simply called route.

brewkode and others added 3 commits October 2, 2016 17:18
RequestForwarder => Router + other such renaming
Also fixed a bug in the Router Interceptor when forwarding request
override def onReady(): Unit = delegate.onReady()
override def onMessage(incomingRequest: ReqT): Unit = {
// TODO - Handle forwarding loop here
if(routingStrategy.route.isDefinedAt(incomingRequest)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure how this helps.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It helps when you don't want to forward certain kind of requests like health / ping. I know we can choose not to add this intercept for them, but I felt this also gives greater power in the hands of the developer. On the other hand it could also be deemed as confusing because of more than 2 ways to do the same thing. What do you think we should be doing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. We should then use applyOrElse as that seems a better way to do this (from the scaladoc)

Note that expression pf.applyOrElse(x, default) is equivalent to {{{ if(pf isDefinedAt x) pf(x) else default(x) }}} except that applyOrElse method can be implemented more efficiently.

@codecov-io
Copy link

codecov-io commented Oct 2, 2016

Current coverage is 58.90% (diff: 55.31%)

Merging #11 into master will increase coverage by 6.64%

@@             master        #11   diff @@
==========================================
  Files            11         14     +3   
  Lines           155        219    +64   
  Methods           0          0          
  Messages          0          0          
  Branches         26         34     +8   
==========================================
+ Hits             81        129    +48   
- Misses           68         81    +13   
- Partials          6          9     +3   

Powered by Codecov. Last update 0a50967...e2bba12

@ashwanthkumar
Copy link
Owner Author

I think we should merge this branch soon. I've experimented too much stuff in here. I'm very very happy with how things have progressed so far 👍

@brewkode brewkode merged commit 5e4f663 into master Oct 3, 2016
@brewkode brewkode deleted the handle-or-forward branch October 3, 2016 05:11
@ashwanthkumar ashwanthkumar modified the milestone: 0.1 Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants