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

Add wrappers for Finagle HTTP #4

Merged
merged 20 commits into from
Dec 18, 2014
Merged

Add wrappers for Finagle HTTP #4

merged 20 commits into from
Dec 18, 2014

Conversation

bguthrie
Copy link
Contributor

@bguthrie bguthrie commented Dec 5, 2014

This pull request adds preliminary support for Finagle HTTP. It provides constructors and Clojure-friendly wrappers for Server, Request, and Response objects, and basic ServerBuilder support. It does not yet attempt to wrap all possible getters and setters.

([v]
(Option/apply v)))

(defn empty? [^Option o]
Copy link
Collaborator

Choose a reason for hiding this comment

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

the docstring for the empty? & get fns should come before the argument list, otherwise codox won't pick up the docstring.

@samn
Copy link
Collaborator

samn commented Dec 6, 2014

Overall this is really awesome @bguthrie!

One thing to think about is the use of ServerBuilder vs the Finagle 6 codec APIs.
http://twitter.github.io/finagle/guide/FAQ.html#how-do-i-configure-clients-and-servers-with-finagle-6-apis

Many configuration items that used to be on Server/Client Builder are now configured through stack params on the codec.

I'd rather add wrappers around the more forward compatible Codec APIs but hate to throw out the work you've already done. What do you think? I can start adding a wrapper for stack params if you want.

@bguthrie
Copy link
Contributor Author

bguthrie commented Dec 7, 2014

Thanks for the comments!

I guess I could go either way with the ServerBuilder stuff. I've been working with stack params for the MySQL client and after futzing with them for a while I'm still not entirely convinced I understand how they work or what the interop from the Java side is supposed to look like. All else being equal I'd rather use the newer API. That said, I don't think this one has been actively deprecated.

@bguthrie
Copy link
Contributor Author

bguthrie commented Dec 7, 2014

The CI build seems to be failing on the 1.4 branch specifically, perhaps related to the addition of the clj-http library. Is there a reason for the 1.4 support?

@bguthrie
Copy link
Contributor Author

bguthrie commented Dec 8, 2014

Rather than try to fix the build using clj-http I'll try using the Finagle HTTP client and see if we can get that going.

@samn
Copy link
Collaborator

samn commented Dec 8, 2014

i've maintained 1.4 compatibility so this can be used with Storm (which unfortunately is locked at 1.4)

Is there an older version of clj-http that you could use?

@bguthrie
Copy link
Contributor Author

I've removed clj-http, added support for Finagle 6-style clients and servers, and upgraded to Finagle 6.24 in line with master. I've also supplemented the ServerBuilder support with an extensive set of functions for working with ClientBuilders; I'd like to suggest retaining Builder support, since it's not officially deprecated and some stacks (like ours) may rely on access to it. Testing still needs to be improved, but please look this over and let me know if you have any other comments apart from that.

(m/set-content-string "Hello, World")))))

(facts "about the HTTP server"
(let [s (server/serve ":3000" hello-world)
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to be closed after the test runs? otherwise successive runs through lein midje :autotest will fail since the port will already be bound

@bguthrie
Copy link
Contributor Author

Fixed! Sorry, good catch––wasn't running with :autotest. Everything looks okay now.

@samn
Copy link
Collaborator

samn commented Dec 17, 2014

one last thing! your commits with some->> failed CI https://travis-ci.org/finagle/finagle-clojure/builds/44273584

i'd like to maintain Clojure 1.4 compatibility since Storm is locked in at that version (and my main production usage of finagle-clojure uses Storm). Is that ok with you? I like some->> which makes 1.4 kind of a bummer.

@bguthrie
Copy link
Contributor Author

Sorry! Saw that earlier and I thought I'd fixed it on the last push. I totally hear you on 1.4 compatibility. I'm just using some->/some->> in test code; not a big deal. Pushing an update now.

@bguthrie bguthrie changed the title Add support for Finagle HTTP servers Add wrappers for Finagle HTTP Dec 17, 2014
@samn
Copy link
Collaborator

samn commented Dec 18, 2014

this is really awesome, thanks @bguthrie!

samn added a commit that referenced this pull request Dec 18, 2014
@samn samn merged commit b0c2027 into finagle:master Dec 18, 2014
@bguthrie
Copy link
Contributor Author

👍 Awesome. Thanks for all the great feedback! MySQL on the way soon.

@bguthrie bguthrie deleted the add_support_for_finagle_http branch December 18, 2014 16:43
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.

None yet

2 participants