(SERVER-391) Throw slingshot exception on timeout #505
Conversation
dankreek
force-pushed
the
SERVER-391/provide-better-error-message-for-borrow-timeout
branch
from
0b32372
to
a0b2310
Compare
| (let [path (get-route this) | ||
| config (get-config) | ||
| certname (get-in config [:puppet-server :certname]) | ||
| localcacert (get-in config [:puppet-server :localcacert]) |
There was a problem hiding this comment.
extra spaces in the get-in args on these two lines
| [puppetlabs.services.jruby.jruby-puppet-service :as jruby] | ||
| [puppetlabs.trapperkeeper.services :as tk-services])) | ||
| [clojure.tools.logging :as log])) | ||
|
|
||
| (defn- handle-request |
There was a problem hiding this comment.
This whole function should be removed now, right, since nothing should be calling it?
|
👍 other than egregious Wolfe Principle™ violations. |
|
All Wolfe Principle violations have been addressed. |
|
👍 @KevinCorcoran, you good with this? |
|
Sorry y'all, I won't have time to look at this until next Thursday. But you can feel free to merge it without waiting on me, I'm satisfied that we got to the bottom of the namespaced-keyword mystery. 🔍 |
|
Well, here it is "next Thursday" and we still haven't merged this. @KevinCorcoran - you still have time to take a quick pass at this and see if it looks reasonable to merge? |
| [request jruby-service config] | ||
| (jruby/with-jruby-puppet jruby-puppet jruby-service | ||
| (core/handle-request request jruby-puppet config))) | ||
|
|
There was a problem hiding this comment.
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 jruby-puppet instance here, instead of passing the JRuby service reference down into handle-request.
There was a problem hiding this comment.
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. request-handler-core). Makes it a bit more onerous to test, etc.
There was a problem hiding this comment.
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.
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.
I misunderstood what was going on here; this LGTM.
…-message-for-borrow-timeout (SERVER-391) Throw slingshot exception on timeout
No description provided.