Code review comment for lp:~pbeaman/akiban-persistit/fix-1021734-nightly-deadlock

Revision history for this message
Nathan Williams (nwilliams) wrote :

Some of the changes are delicate (new claim strategy, delayed deallocation) but I think make sense. Just a few questions and suggestions below.

Buffer#setLongRecordPointer() - exits quietly if not a data page. Should that be an error instead?

Buffer#addGarbageChain() - New code is searching through all chains and asserting the first isn't the same as the one being added? Not questioning it, just checking my understanding.

SharedResource#isOther() - This checks that a) there is no writer or b) I am the writer, right? Could we javadoc that or change the name (e.g. isMineOrNone())?

Might be useful to add some messages to the new asserts in case they ever fire. For example, the left page addr if right page is -1 in Buffer#allocPage().

Did you change the Jenkins job to run the stress tests with asserts?

review: Needs Information

« Back to merge proposal