Skip to content

Conversation

rlinehan
Copy link
Contributor

@rlinehan rlinehan commented Jun 9, 2016

Both the pool context and the instance contain keys that we don't want users
to mess with - such as the pool and flush agents and the flush-instance-fn.
This commit moves all keys we want to keep "private" in both the pool context
and the instance underneath an :internal key.

For the the pool context, the :config key remains public; for the instance,
the :id and :scripting-container remain public.

Both the pool context and the instance contain keys that we don't want users
to mess with - such as the pool and flush agents and the `flush-instance-fn`.
This commit moves all keys we want to keep "private" in both the pool context
and the instance underneath an `:internal` key.

For the the pool context, the `:config` key remains public; for the instance,
the `:id` and `:scripting-container` remain public.
@cprice404
Copy link

+1

@@ -85,13 +93,14 @@
old-pool-state :- jruby-schemas/PoolState
new-pool-state :- jruby-schemas/PoolState
refill? :- schema/Bool]
(let [{:keys [config pool-state]} pool-context
(let [{:keys [config]} pool-context
pool-state-atom (get-in pool-context [:internal :pool-state])
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to call (jruby-internal/get-pool-state pool-context) here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get-pool-state returns the dereferenced atom, but I can use get-pool-state-container, and yeah doing that sounds like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool - makes sense.

@camlow325
Copy link
Contributor

👍 and can merge after the one cosmetic change with the use of get-pool-state-container is merged in, unless we want to hold for @jpinsonault to review first.

@rlinehan
Copy link
Contributor Author

@camlow325 updated.

@camlow325
Copy link
Contributor

Looks like Travis failed on one cell, for oraclejdk7 only, for the latest run on this PR. From https://travis-ci.org/puppetlabs/jruby-utils/jobs/137349890:

lein test puppetlabs.services.jruby.jruby-agents-test

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

Doesn't seem like it would be related to the changes in this PR. @rlinehan, you agree?

@rlinehan
Copy link
Contributor Author

Hmmm, I agree, but also it's failed twice in a row now (I already reran it once).

@jpinsonault
Copy link
Contributor

Well I'm 👍. Are we good with merging (re: travis)?

@rlinehan
Copy link
Contributor Author

@jpinsonault I believe we are. We're going to change some of the awaits to timed-awaits in the namespace that was timing out, but we decided to do that in another PR and not block this one on it.

@jpinsonault
Copy link
Contributor

Cool

@jpinsonault jpinsonault merged commit b367b32 into puppetlabs:master Jun 13, 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.

4 participants