Diffa Diffa → 0.9.3

Comments and changes to this ticket

• 

  Ben Hood October 14th, 2010 @ 11:13 PM

  • State changed from “new” to “open”
  • Milestone cleared.
  • Assigned user set to “Ben Hood”
  • Milestone order changed from “21” to “0”

  This seems to be a race condition during the insertion. In the demo app, the entity with id id1 has:

  • fea80f2db003d4ebc4536023814aa885 as an upstream version
  • 31b7e2ba3c498e01a2c22fd4c701d762 as a downstream version

  Both {storeUpstreamVersion,storeDownstreamVersion} query the Correlation table to see whether they should update or insert new. But when the digests are entered into the database in different threads, the 2nd thread creates an insert into the Correlation table rather than an update as shown by this log output:

  23:03:24.528 [qtp967999600-22] HibernateVersionCorrelationStore.scala:64 - Storing down: VersionID(WEB-1,id1) dvsn = 31b7e2ba3c498e01a2c22fd4c701d762 uvsn = 31b7e2ba3c498e01a2c22fd4c701d762
  23:03:24.528 [qtp967999600-24] HibernateVersionCorrelationStore.scala:41 - Storing up: VersionID(WEB-1,id1) vsn = fea80f2db003d4ebc4536023814aa885
  23:03:24.546 [qtp967999600-24] HibernateVersionCorrelationStore.scala:47 - DB Insert: fea80f2db003d4ebc4536023814aa885
  

• 

  Ben Hood October 15th, 2010 @ 01:32 PM

  So after a discussion with Paul, we have come to the conclusion that the whole correlation store is subject to race conditions. In this particular example the update or insert (upsert) requires a check to see if a row with the same key information already exists. This is very timing sensitive when the kernel is running multiple threads. So the conclusion that we're drawing is the following:

  • We need a unique constraint in the database to identify this sort of bug earlier and prevent the insertion
  • The storing of versions should be handled by an actor that is scoped on a pair key. This is effectively a serialization barrier to prevent concurrent modification.
  • We would also need to give consideration of how this could be clustered, should the requirement ever transpire down the line.
• 

  Ben Hood October 17th, 2010 @ 08:11 PM

  (from [1297373c86eaeb1d72f2ae37428a4035e74a492c]) First cut of protecting the unsafe correlation store calls with actors [[#137]] http://github.com/lshift/diffa/commit/1297373c86eaeb1d72f2ae37428a4...

• 

  Ben Hood October 17th, 2010 @ 08:15 PM

  I've pushed an initial akka based solution of this onto a bug branch - so that it can be peer QA'ed. There are still some cleanups that need to go on:

  • Split the VersionCorrelationStore trait into safe and unsafe traits so that clients don't make the mistake of by-passing the actor
  • Add actor specific mock tests
  • Consider using an akka supervisor and link each actor with some kind of restart strategy
• 

  Ben Hood October 17th, 2010 @ 09:45 PM

  (from [239a4e4abb411ce1ca7c83cd673eaac56aac49b8]) Added unit test for correlation actor [[#137]] http://github.com/lshift/diffa/commit/239a4e4abb411ce1ca7c83cd673ea...

• 

  Ben Hood October 18th, 2010 @ 11:13 AM

  Synopsis of a QA discussion with Paul:

  • Serializing from with a policy implementation is too fine grained - it should happen in the ChangesHandler
  • The VersionPolicy trait should demarcated as an unsafe trait that requires serialization - this obviates the need to split the CorrelationStore into safe and unsafe traits
  • Convenience calls to the Actor object should be scoped explicitly for avoidance of confusion
• 

  Ben Hood October 18th, 2010 @ 07:19 PM

  (from [e21a0a1e0f424e1ddfd1e7ff3b57559cf2f00e02]) Made the VersionPolicy trait thread-unsafe inline with QA discussion, pulled all of the other abstractions around this [[#137]] http://github.com/lshift/diffa/commit/e21a0a1e0f424e1ddfd1e7ff3b575...

• 

  Ben Hood October 19th, 2010 @ 09:06 AM

  (from [dde03007c3adb1f37adf78a56a2efc5fdda69c96]) Turned a try/catch into an Option [[#137]] http://github.com/lshift/diffa/commit/dde03007c3adb1f37adf78a56a2ef...

• 

  Ben Hood October 19th, 2010 @ 09:59 AM

  (from [af0c978a17855271096214407501af74c3c92837]) Pulled actor test straight [[#137]] http://github.com/lshift/diffa/commit/af0c978a17855271096214407501a...

• 

  Ben Hood October 19th, 2010 @ 10:19 AM

  (from [9d03106010fec288a525fcde80fcc7b0c6e5b21c]) Added some documentation, moved repetitive call to constructor [[#137]] http://github.com/lshift/diffa/commit/9d03106010fec288a525fcde80fcc...

• 

  Ben Hood October 19th, 2010 @ 10:19 AM

  (from [5f2b9c9dab14e4f4513a825b0f7d6dce618ddeff]) The policy tests require no knowledge of any actors [[#137]] http://github.com/lshift/diffa/commit/5f2b9c9dab14e4f4513a825b0f7d6...

• 

  Ben Hood October 19th, 2010 @ 10:35 AM

  (from [8d1f1a2cd25b894bed3ea9916da388cb134a656b]) Pushed the participant resolution up so that less uneccesary dependencies such as the ConfigStore are pulled in [[#137]] http://github.com/lshift/diffa/commit/8d1f1a2cd25b894bed3ea9916da38...

• 

  Ben Hood October 19th, 2010 @ 10:48 AM

  (from [e659af219c8e50f3056272510ae1e92ae43884cb]) Stop the current actor when updating a pairing [[#137]] http://github.com/lshift/diffa/commit/e659af219c8e50f3056272510ae1e...

• 

  Ben Hood October 19th, 2010 @ 11:04 AM

  (from [4f00869c1d06b85b658c49dae5b73391f6c36b77]) Minor code cleanup [[#137]] http://github.com/lshift/diffa/commit/4f00869c1d06b85b658c49dae5b73...

• 

  Ben Hood October 19th, 2010 @ 11:12 AM

  • State changed from “open” to “resolved”
  • Assigned user changed from “Ben Hood” to “Paul Jones”

  This looks like it's worth QA'ing now. One ex-ante comment I have is that I didn't put a restarting supervisor to manage crashing actors as I didn't think this was strictly necessary for the scope of this bug, but it is a feature we could consider later on.

• 

  Paul Jones October 19th, 2010 @ 12:28 PM

  • State changed from “resolved” to “open”
  • Assigned user changed from “Paul Jones” to “Ben Hood”

  Next round of QA comments:

  • Is there a point to the ActorMessage case class? It doesn't seem to be extended, and I can't see it used anywhere;
  • To make the naming more consistent, what about naming the ChangeEventClient as PairClient (or similar)? I wouldn't necessarily class a sync action as a change event, so the current name isn't quite all-encompassing;
  • Perhaps make withActor a currying function, and then dump handleMessage in favour of using it inline (since as far as I can see, handleMessage is only used once);
  • Apart from not having to define @ThreadSafe yourself, what is the value of using @ThreadSafe from jcip?
  • I'd suggest similar currying for withCurrentPair in Configuration.scala.
  • The current style has been not to put the code blocks in braces for inline statements - was there a reason for doing so in the call to withCurrentPair for stopping actors?
  • What starts all the persistent actors when the agent starts up? I can't seem to find the code path that does so.
• 

  Ben Hood October 19th, 2010 @ 01:28 PM

  (from [036e6dc5e11ec44c52f1b76483043ce2eebfa238]) Simplified lambda [[#137]] http://github.com/lshift/diffa/commit/036e6dc5e11ec44c52f1b76483043...

• 

  Ben Hood October 19th, 2010 @ 01:28 PM

  (from [649c7a7959117d295adf3dfae92aa9da952b9b98]) Code convention [[#137]] http://github.com/lshift/diffa/commit/649c7a7959117d295adf3dfae92aa...

• 

  Ben Hood October 19th, 2010 @ 01:28 PM

  (from [8f68b55788dbadd344d41c407241d61ec9cbefbc]) Deleted redundant case class [[#137]] http://github.com/lshift/diffa/commit/8f68b55788dbadd344d41c407241d...

• 

  Ben Hood October 19th, 2010 @ 01:28 PM

  • Assigned user changed from “Ben Hood” to “Paul Jones”

  • ActorMessage: Agreed, I've deleted this.
  • Naming convention: I originally called it PairClient but dropped it because that name didn't sound right either. ChangeEventClient is not much better, for the reason you've mentioned. Will have to rethink.
  • withActor: I simplified this.
  • @{NotThreadSafe,ThreadSafe}: The only other value is the lines of documentation they've written about this (http://www.javaconcurrencyinpractice.com/annotations/doc/net/jcip/a...) and the fact that this annotation is described and used elsewhere.
  • withCurrentPair: Not quite sure how a curry will simplify this - maybe you can suggest?
  • inline code blocks: fixed
  • persistent actors: Nothing currently brings them up - I wonder what the best place to do this is (in the constructor of the Configuration class perhaps)?
• 

  Ben Hood October 19th, 2010 @ 02:47 PM

  How about renaming the ChangeEventClient to VersionPolicyClient?

• 

  Paul Jones October 19th, 2010 @ 02:49 PM

  • Assigned user changed from “Paul Jones” to “Ben Hood”

  That's better, but the name still doesn't match up with the name of the actor, to which it is the actual client.

• 

  Paul Jones October 19th, 2010 @ 02:50 PM

  • Assigned user changed from “Ben Hood” to “Paul Jones”

  Oops, just noticed the previous comment that was saying the QA is done.

• 

  Paul Jones October 19th, 2010 @ 02:57 PM

  • Assigned user changed from “Paul Jones” to “Ben Hood”

  • withCurrentPair: The suggestion was just a stylistic like of mine, since in the case of no-args methods, it would allow the nicer block syntax to be used. With a arg though, its of questionable value. It does however dis-courage the actual callback method from being bumped from being the final parameter;
  • What about making the supervisor do it? This would then mirror the LocalEventMatcherManager behaviour.
• 

  Ben Hood October 19th, 2010 @ 03:14 PM

  How about PairPolicyClient? PerPairPolicyClient?

• 

  Ben Hood October 19th, 2010 @ 03:20 PM

  I guess the recovery can't run earlier than the boot time of the
  supervisor, so it seems like a good spot. It brings in the ConfigStore
  as a dependency into the supervisor but it could be wired by some
  Spring post-initialize routine so that it doesn't have to go into the
  constructor.

• 

  Paul Jones October 19th, 2010 @ 03:24 PM

  For the client name, the above or "PairAction" client would be fine. I think I'd be mostly good if it contains the word pair.

  Why can't the ConfigStore go into the constructor? It isn't like it would be a circular dependency, and having multiple injection methods adds complexity.

• 

  Ben Hood October 19th, 2010 @ 03:28 PM

  On reflection, I guess the ConfigStore could go into the constructor, so we'll just go with that then.

• 

  Ben Hood October 19th, 2010 @ 03:52 PM

  (from [ac935381187dc21195a910af77a4853c841c149b]) QA remarks: 1. Actor recovery when agent is booted
  2. Renamed the ChangeEventClient
  [[#137]] http://github.com/lshift/diffa/commit/ac935381187dc21195a910af77a48...

• 

  Ben Hood October 19th, 2010 @ 03:55 PM

  • State changed from “open” to “resolved”
  • Assigned user changed from “Ben Hood” to “Paul Jones”

  OK, resolving again after fixing the recovery and naming issues.

  If you think the currying still adds value, then free to add this as part of the merge - re-assign to me if you think I should do it, but I'm not seeing where it makes things terser in this example - but you may have an idea.

• 

  Ben Hood October 28th, 2010 @ 04:23 PM

  • State changed from “resolved” to “closed”

  [bulk edit]

• 

  Ben Hood February 4th, 2011 @ 02:48 PM

  • Milestone set to “0.9.3”
  • Milestone order changed from “3” to “0”

  [bulk edit]