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
(SERVER-391) Throw slingshot exception on timeout #505
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,28 @@ | |
(:require [puppetlabs.trapperkeeper.core :as tk] | ||
[puppetlabs.services.protocols.request-handler :as handler] | ||
[puppetlabs.services.request-handler.request-handler-core :as core] | ||
[puppetlabs.trapperkeeper.services :as tk-services] | ||
[puppetlabs.services.jruby.jruby-puppet-service :as jruby] | ||
[puppetlabs.trapperkeeper.services :as tk-services])) | ||
[clojure.tools.logging :as log])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this shouldn't be needed either if |
||
|
||
(defn- handle-request | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole function should be removed now, right, since nothing should be calling it? |
||
[request jruby-service config] | ||
(jruby/with-jruby-puppet jruby-puppet jruby-service | ||
(core/handle-request request jruby-puppet config))) | ||
[request jruby-service config puppet-version] | ||
(try | ||
(jruby/with-jruby-puppet jruby-puppet jruby-service | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you make this change? I try to keep interactions with services contained within "service" namespaces - which was the origin reason for getting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's a convention/style thing, but I find it a bit odd to see service references being passed around into namespaces that otherwise have nothing to do with Trapperkeeper. (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, @dankreek and I talked about that and waffled a bit. I had found it a bit odd to be doing more seemingly domain specific logic like getting a jruby-puppet instance and possibly construct a ring response from the service namespace rather than core, which I kind of thought was where @cprice404 was on this as well. I don't have a strong opinion on this, though - other than that I want to get this PR in soon so we can move on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, in this case we either err on the side of putting a "service" thing in core code, or do core things in service code. This solution ended up being the least ugly, in my opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I misunderstood what was going on here; this LGTM. |
||
(core/handle-request request jruby-puppet config)) | ||
(catch IllegalStateException e | ||
(let [message (.getMessage e)] | ||
(log/error message) | ||
{:status 503 | ||
:body message | ||
:headers {"Content-Type" "text/plain" | ||
"X-Puppet-version" puppet-version}})))) | ||
|
||
(tk/defservice request-handler-service | ||
handler/RequestHandlerService | ||
[[:PuppetServerConfigService get-config]] | ||
[[:PuppetServerConfigService get-config get-in-config]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need to add this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
(handle-request | ||
[this request] | ||
(let [jruby-service (tk-services/get-service this :JRubyPuppetService) | ||
config (core/config->request-handler-settings (get-config))] | ||
(handle-request request jruby-service config)))) | ||
(core/handle-request request jruby-service config)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic - extra line?