Code review comment for lp:~pbeaman/akiban-persistit/fix_1018526_temp_trees_in_journal

Revision history for this message
Peter Beaman (pbeaman) wrote :

Responses to Nathan:

What is the intended lifetime of the tree handle now? It looks like it will get created on remove key, remove tree, store key, and accumulator delta inside of a transaction. -- Correct

It looks like _ignoreTransactions hould always mirror isTemporary but it is up to the caller. - Not quite: currently the directory tree exchange operates with _ignoreTransactions on non-temporary volumes.

Maybe an assert inside of JournalManager#handleForTree() would be nice. - Added

A couple of the checks for handle != 0 are also slightly confusing. We shouldn't have MVVs in a temporary tree (txns being ignored) so if we are pruning, we should always have a handle. - Correct. Changed to asserts.

Similarly, how could we get an index hole if they can't be shared across threads and aren't recovered? - There's an obscure, gnarly re-balance case in raw_removeKeyRangeInternal that can leave an index hole. A fix for another bug may remove it, but for now since we think it can happen, the condition in walkRight should be retained.

The new test with the "upper bound" to avoid a checkpoint race. We already have a couple intermittent test failures, it would be nice to structure this so it is always deterministic. - Yes. I suggest we should do that comprehensively in a different branch.

« Back to merge proposal