Diffa Diffa → 0.9.3

Comments and changes to this ticket

• 

  Paul Jones February 13th, 2011 @ 04:16 PM

  • no changes were found...
• 

  Ben Hood February 14th, 2011 @ 07:28 AM

  Sounds plausible - have you seen this behavior in practice or is just a suspicion at this point?

  Do you think it's worth drawing a timeline diagram that shows such a cause of events?

  In any case, I think the place to start is developing some test scenarios to provoke this behavior so that we can demonstrate that the fix actually works.

• You flagged this item as spam.
  

  John Wright March 4th, 2011 @ 03:17 PM

  • State changed from “new” to “resolved”
  • Assigned user set to “Paul Jones”

  Seeing a 20x - 40x (!) performance boost in the VersionCorrelationStorePerfTest.

  https://github.com/johnwright/diffa/commits/188

• 

  Paul Jones March 4th, 2011 @ 04:56 PM

  • State changed from “resolved” to “open”
  • Assigned user changed from “Paul Jones” to “John Wright”

  QA notes:

  Overall, change looks good - certainly ended up being less code than I would have expected.

  Changes I'd like to see:

  • Fix merge conflict with master;
  • deletedDocs needs to be cleared when you flush the session. Given I only just noticed this at the minute, it might be a good idea to add some tests to validate that flushing puts the session back into a ready state when its complete (ie, do stuff, flush, do more stuff that might conflict, flush);
  • Can we consider adding a bound on the growth of updatedDocs + deletedDocs? If we don't, then a huge sync could run us out of memory;
  • Can we add protection around adding and removing in the same session? Even better, add a log message and an implicit flush (so you see a performance drop, but not a complete break);

• You flagged this item as spam.
  

  John Wright March 7th, 2011 @ 02:04 PM

  • State changed from “open” to “resolved”
  • Assigned user changed from “John Wright” to “Paul Jones”

  Updated for QA notes.

  https://github.com/johnwright/diffa/commits/188

• 

  Paul Jones March 7th, 2011 @ 09:27 PM

  • State changed from “resolved” to “open”
  • Assigned user changed from “Paul Jones” to “John Wright”

  For completeness, further QA remarks discussed:

  • Flush isn't necessary internally with the prepareUpdate and prepareDelete methods - cleaning up the session state is enough;
  • Session is an overloaded term - Writer will likely cause much less confusion;
• 

  Ben Hood March 8th, 2011 @ 02:58 AM

  Forgive me if I haven't been part of the latest conversation, so some of my comments may be out of context:

  • When the PairActor receives a Difference command, it issues a flush to the writer and then invokes the policy. Internally the policy also issues a flush command. Is this double flush intentional? It seems that this is the only reason why the writer is passed into the policy API.

  • LuceneWriter:

    • Why is the maxBufferSize hard coded to 10K? Would you never want to change this?
    • flush/0: If performance were a concern, would it be sensible to demote the log call to the trace level instead of debug?

  • Would it be too difficult to write a test for the Scheduler in the LuceneVersionCorrelationStoreTest, seeing as there are tests for the buffer size flush?

  • Performance testing: I've seen you've mentioned a speed up in the performance test - does this still hold after all of the QA changes?

  • Session vs. Writer nonemclature - I agree with the point that session is an oveloaded term, however, by the same logic, does the same not apply to Writer? What about calling it something like IngressBatcher (just lifted from the title of this ticket) or something along the lines of Buffer/BufferedEventWriter/etc?

• 

  Paul Jones March 8th, 2011 @ 07:00 AM

  Flushing:

  • The onChange needs to be passed the writer in order to be able to contribute to the buffer. Since the differencing operation also needs access to a writer, it didn't seem too big an issue to pass that in. The flush beforehand is to ensure that any pending change events are written. The flush internally is to get the changes collected during the sync ready for the unmatched version query.

  LuceneWriter:

  • I suggested this could be hardcoded at this stage, save exposing too many tuning knobs at this point;
  • There are probably quite a lot of places we should consider doing that - the digest builder is also exceedingly chatty;

  Nomenclature:

  • I felt that calling the upper API a writer was somewhat aligning the two pieces. Session was also already a concept within the Diffa domain, whereas writer is only overloaded with respect to the backing implementation.

• You flagged this item as spam.
  

  John Wright March 8th, 2011 @ 11:28 AM

  • State changed from “open” to “resolved”
  • Assigned user changed from “John Wright” to “Paul Jones”

  • Logging in writer changed to trace level
  • Added a small test for scheduled flushes

  https://github.com/johnwright/diffa/commits/188

• 

  Ben Hood March 8th, 2011 @ 01:31 PM

  (from [82e0ce996f1e24ae315d1ab219f60db0961f96c0]) Merge [#188] into master https://github.com/lshift/diffa/commit/82e0ce996f1e24ae315d1ab219f6...

• 

  Ben Hood March 8th, 2011 @ 01:31 PM

  (from [6a4ebdaeb8683b263f9e7a7dbf2454fe2d108520]) [#188] Fix cleanup of store for perf test https://github.com/lshift/diffa/commit/6a4ebdaeb8683b263f9e7a7dbf24...

• 

  Paul Jones March 8th, 2011 @ 01:31 PM

  • State changed from “resolved” to “closed”

  QA+