(SERVER-150) flush jruby instances #313

cprice404 · 2014-12-05T18:50:56Z

No description provided.

This commit gets rid of our use of a raw `future` when priming the JRuby pool. Instead, we use a clojure agent. This has the following benefits: * Will make it easier to re-use the pool initialization logic * When we expose a way to request for the pool to be flushed, will make it possible to prevent multiple pool initializations to occur simultaneously * Gives us more robust options around error handling, since we could now call `agent-error` on the agent instance to make sure that nothing has gone wrong * Provides some scaffolding that we can re-use in order to have another agent handle the flushing of an existing pool

The previous implementation of `prime-pool!` required a `PoolContext` object as input. This meant that the function could not be used to prime a pool that was not yet part of a `PoolContext`. This commit refactors the function so that it operates on a `PoolStateContainer` instead; this will allow us to initialize a pool before putting it into the `PoolContext`. Conflicts: src/clj/puppetlabs/services/jruby/jruby_puppet_core.clj src/clj/puppetlabs/services/jruby/jruby_puppet_service.clj

Prior to this commit, the `borrow-instance` and related functions in jruby-core required you to pass a `PoolContext` object in as input. This prevented us from using these functions on a pool that has not yet been added to the context. This commit modifies the signatures to accept the pool object directly, rather than the `PoolContext`. Conflicts: src/clj/puppetlabs/services/jruby/jruby_puppet_service.clj

Prior to this commit, the various `return` functions that would allow a caller to return a JRuby instance to a pool required that you also pass in a reference to the pool that the instance should be returned to. This allowed the possibility of returning an instance to a pool that it did not actually originate from. This commit refactors the creation/borrow/return logic such that an instance maintains a reference to the pool that it was borrowed from, and the `return` functions use that reference when returning the instances to a pool. This guarantees that instances will always be returned to the correct pool that they were originally borrowed from.

This commit changes the JRubyPuppetInstance schema from a normal, map-based schema to a schematized `defrecord`. This may result in a very minor performance improvement, but the main motivation is to provide better compatibility with REPL-based developer utility code. Since we added a reference to the JRuby pool to the schema for JRubyPuppetInstance, attempting to print the objects results in a stack overflow, because Clojure's implementation of `toString` for maps will recurse into these references to the pools and attempt to print all of the instances again. Moving to `defrecord` would allow us to override `toString` if we wished to do so, but the default implementation of `toString` for a record does not have the same problem with recursion that the map objects do, so simply converting to a record solves the issue and allows all of the utility code to be used in the REPL again.

This commit adds a new service function to the JRubyPuppet service. The new function can be used to request a flush of the JRuby pool. The function is implemented by creating a new (offline) JRuby pool and starting to populate it; as soon as the first instance is ready, we can swap it into the service and remove the old pool, and then continue initializing the remaining instances and cleaning up the old pool in parallel. It uses clojure agents to coordinate the creation of the new pool and the cleanup of the old pool.

KevinCorcoran · 2014-12-05T22:34:31Z

src/clj/puppetlabs/services/jruby/jruby_puppet_agents.clj

@@ -0,0 +1,100 @@
+(ns puppetlabs.services.jruby.jruby-puppet-agents
+  (:import (clojure.lang IFn Agent)
+           (com.puppetlabs.puppetserver PuppetProfiler)


This import is unused.

When we replace the old pool with the new pool during a 'flush' request, we need to have a way to handle the possible race condition where some code has a reference to the old pool and tries to borrow an instance from it after it has already been flushed. This commit is the first step towards that; it puts a RetryPoisonPill into the old queue.

KevinCorcoran · 2014-12-05T22:37:45Z

src/clj/puppetlabs/services/jruby/jruby_puppet_agents.clj

+(ns puppetlabs.services.jruby.jruby-puppet-agents
+  (:import (clojure.lang IFn Agent)
+           (com.puppetlabs.puppetserver PuppetProfiler)
+           (puppetlabs.services.jruby.jruby_puppet_core PoisonPill RetryPoisonPill))


This import looks like it overlaps with a the require below.

imports are different from requires. One is importing Java classes, one is importing the clojure namepsace.

This commit updates the jruby-service `with-jruby-puppet` macro to check for the RetryPoisonPill, and adds corresponding tests.

cprice404 · 2014-12-08T16:35:28Z

test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj

+          (is (jruby-core/retry-poison-pill? old-pool-instance)))))))
+
+(deftest with-jruby-retry-test-via-fake-pool
+  (testing "with-jruby-puppet retries if it encounters a RetryPoisonPill"


We probably don't need both this test and...

KevinCorcoran · 2014-12-08T22:12:47Z

test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj

+        ; wait until the flush is complete
+        (await (:pool-agent context))
+        (let [old-pool-instance (jruby-core/borrow-from-pool old-pool)]
+          (is (jruby-core/retry-poison-pill? old-pool-instance)))))))


I think it is possible to simplify this test by simply removing both the promise and the watch ...

diff --git a/test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj b/test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj index 2de1591..ba6411a 100644 --- a/test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj +++ b/test/unit/puppetlabs/services/jruby/jruby_puppet_agents_test.clj @@ -75,18 +75,10 @@ (let [jruby-service (tk-app/get-service app :JRubyPuppetService) context (tk-services/service-context jruby-service) pool-context (:pool-context context) - old-pool (jruby-core/get-pool pool-context) - pool-state-swapped (promise) - pool-state-watch-fn (fn [key pool-state old-val new-val] - (when (not= (:pool old-val) (:pool new-val)) - (remove-watch pool-state key) - (deliver pool-state-swapped true)))] + old-pool (jruby-core/get-pool pool-context)] ; borrow an instance so we know that the pool is ready (jruby/with-jruby-puppet jruby-puppet jruby-service) - (add-watch (:pool-state pool-context) :pool-state-watch pool-state-watch-fn) (jruby-protocol/flush-jruby-pool! jruby-service) - ; wait until we know the new pool has been swapped in - @pool-state-swapped ; wait until the flush is complete (await (:pool-agent context)) (let [old-pool-instance (jruby-core/borrow-from-pool old-pool)]

Basically, I think await takes care of everything for you, and I think the promise + watch are essentially redundant. But I could be wrong ...

I think you're right. I think that in an earlier incarnation of this test, I was trying to do something in between the promise and the await, but I'm obviously not any more. I'll play with it, but I think you're right.

Word, yeah, I applied that patch and ran the test a bunch and it was all green.

KevinCorcoran · 2014-12-08T22:21:36Z

📈 Looks very good overall!

camlow325 · 2014-12-08T22:57:58Z

src/clj/puppetlabs/services/jruby/jruby_puppet_agents.clj

+                   id count)
+        (try
+          (jruby-core/create-pool-instance! new-pool id config profiler)
+          (jruby-core/mark-as-initialized! pool-state)


Can't remember exactly what we were using this for but should this have only been set after all pool instances are successfully initialized?

there is a new JIRA in the backlog to get rid of the initialized? flag altogether

In the original usage it does it in a loop after every new instance is initialized, but I don't remember wtf it is for either... so I was just trying to duplicate that behavior.

camlow325 · 2014-12-09T00:55:23Z

👍 pending,

Question on whether the try/catch block for the PoisonPill creation should wrap around any terminate Exceptions.
Removing the extra with-jruby-puppet if it isn't being used anywhere (hopefully) else somehow deal with the RetryPoisonPill if one is found?
Delete the first of the two retry tests, if desired.
@KevinCorcoran's comments
Travis passage (hopefully)

camlow325 · 2014-12-12T16:27:03Z

Linked Travis job failed, https://travis-ci.org/puppetlabs/puppet-server/jobs/43431114, but appears to be due to a presumably transient error trying to fetch a gem from rubygems -- nothing that the work on this PR should have introduced.

camlow325 · 2014-12-12T16:50:27Z

👍

I thought of one other test that might be useful. I didn't see any new tests that perform concurrent executions of flush-pool! to see that nothing bad happens - i.e., that pool 1 is flushed, pool 2 initialized, pool 2 flushed, pool 3 initialized even if the second call to flush-pool! happens before pool 2 is completely initialized. This would basically just be validating that we're getting the effect from the agent that we expect but might be useful (e.g., for regression, in the event of a future refactor).

I'd be good with this work coming in without this additional test. Will let @cprice404 make the call.

KevinCorcoran · 2014-12-12T17:28:06Z

src/clj/puppetlabs/services/jruby/jruby_puppet_core.clj

    (validate-instance-from-pool! instance pool)))

 (schema/defn ^:always-validate
  return-to-pool
  "Return a borrowed pool instance to its free pool."
-  [context :- PoolContext


Sweet! This is a nice change.

cprice404 · 2014-12-12T18:46:18Z

@camlow325 I like your idea about the multi-flush test; I'll open a ticket for that and do it in a separate PR sometime soon, though, if that's OK with you.

camlow325 · 2014-12-12T18:47:24Z

@cprice404 Yep, I'm good with the extra test landing on a separate ticket and PR.

cprice404 · 2014-12-12T19:45:15Z

@camlow325 I created SERVER-251 to capture the desire for the additional test.

KevinCorcoran · 2014-12-12T19:53:52Z

AFAIK we are ready to 🚢 this sucka.

…h-jruby-instances (SERVER-150) flush jruby instances

Chris Price added 6 commits December 5, 2014 08:55

KevinCorcoran reviewed Dec 5, 2014
View reviewed changes

cprice404 force-pushed the feature/master/SERVER-150-flush-jruby-instances branch from 2786dc3 to 22b4255 Compare December 5, 2014 22:37

KevinCorcoran reviewed Dec 5, 2014
View reviewed changes

(SERVER-150) Check for retry poison pill in borrow macro

0065300

This commit updates the jruby-service `with-jruby-puppet` macro to check for the RetryPoisonPill, and adds corresponding tests.

cprice404 force-pushed the feature/master/SERVER-150-flush-jruby-instances branch from a138993 to 0065300 Compare December 7, 2014 03:09

cprice404 reviewed Dec 8, 2014
View reviewed changes

(SERVER-150) Add basic functionality test for pool flushing

11769a1

KevinCorcoran reviewed Dec 8, 2014
View reviewed changes

camlow325 reviewed Dec 8, 2014
View reviewed changes

MSLilah mentioned this pull request Dec 9, 2014

(FOR REVIEW) (SERVER-151) Flush JRuby Pool endpoint #317

Closed

Chris Price added 3 commits December 8, 2014 20:41

(SERVER-150) Cleanups based on PR feedback

fa2f276

(SERVER-150) Delete unused macro

d7eeebb

(SERVER-150) Remove unused :initialized? state from service context

486baca

KevinCorcoran reviewed Dec 12, 2014
View reviewed changes

MSLilah mentioned this pull request Dec 12, 2014

(SERVER-250) Document jruby-pool Admin endpoint #320

Merged

KevinCorcoran added a commit that referenced this pull request Dec 12, 2014

Merge pull request #313 from cprice404/feature/master/SERVER-150-flus…

2365cfe

…h-jruby-instances (SERVER-150) flush jruby instances

KevinCorcoran merged commit 2365cfe into puppetlabs:master Dec 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SERVER-150) flush jruby instances #313

(SERVER-150) flush jruby instances #313

cprice404 commented Dec 5, 2014

KevinCorcoran Dec 5, 2014

KevinCorcoran Dec 5, 2014

cprice404 Dec 9, 2014

cprice404 Dec 8, 2014

KevinCorcoran Dec 8, 2014

cprice404 Dec 9, 2014

KevinCorcoran Dec 9, 2014

KevinCorcoran commented Dec 8, 2014

camlow325 Dec 8, 2014

KevinCorcoran Dec 8, 2014

cprice404 Dec 9, 2014

camlow325 commented Dec 9, 2014

camlow325 commented Dec 12, 2014

camlow325 commented Dec 12, 2014

KevinCorcoran Dec 12, 2014

cprice404 commented Dec 12, 2014

camlow325 commented Dec 12, 2014

cprice404 commented Dec 12, 2014

KevinCorcoran commented Dec 12, 2014

Navigation Menu

(SERVER-150) flush jruby instances #313

(SERVER-150) flush jruby instances #313

Conversation

cprice404 commented Dec 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinCorcoran commented Dec 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camlow325 commented Dec 9, 2014

camlow325 commented Dec 12, 2014

camlow325 commented Dec 12, 2014

Choose a reason for hiding this comment

cprice404 commented Dec 12, 2014

camlow325 commented Dec 12, 2014

cprice404 commented Dec 12, 2014

KevinCorcoran commented Dec 12, 2014