Paging code review
timfox Nov 21, 2008 4:30 AMAs discussed in our code review yesterday, here are the outstanding issues in paging that need to be addresses.
Some should already be covered in JIRA tasks. If the others don't have JIRAs they need to be covered by at least one, if not fixed immediately.
1. Use OrdereredExecutorFactory for thread management. This should make the global pager redundant.
2. We should ignore corrupt pages on load, and log a clear warning in the logs so the user can address the issue.
3. Much of the functionality currently in PagingManagerImpl is better placed in PagingStoreImpl, e.g. addSize, ondePage. This would also simplify the code since less lookups would be needed and several methods could be removed or made private.
4. PagingStoreImpl - The locking is very complex. Can this be simplified by using Condition objects with the lock? Or perhaps we can disallow concurrent writes to the same page - which would simplify the locking. IS there really much performance to be gained by concurrent writes to the same page? Can this be verified with a test?
5. PagingStoreImpl:
if (returnPage.getNumberOfMessages() == 0)
How can number of messages in a page be zero? Clebert to check
6. PagingStoreImpl::page()
Calculation of block size and lock acquisition:
int bytesToWrite = fileFactory.calculateBlockSize(message.getEncodeSize() + PageImpl.SIZE_RECORD);
// The only thing single-threaded done on paging is positioning and
// check-files (verifying if we need to open a new page file)
positioningGlobalLock.acquire();
Can this be *after* the currentPage check - since in 99% of cases paging is not occurring and we don't want to get locks / do calculations unnecessarily.
7. PagingStoreImpl::page
pagedUsedSize is being incremented twice?
8. TransactionImpl:
An address can switch from paging to not paging mid way during a transaction before it is committed, resulting in the transactions messages being routed out of order
9. TransactionImpl:
Currently paged messages are paged on commit(). If there are a lot of paged messages this could hold up the remoting thread for a significant amount of time,
One of the design principles of the server is each operation should be quick to complete to avoid starving other operations.l
We should consider instead, writing transactional actions straight to a file in the paging case, then on commit, since we know they are on file we can process them on a different thread and the commit can return ok immediately. This would also allow us to deal with huge transactions, much bigger than can fit in memory at once
10. Currently routing code is duplicated in ServerSessionImpl::send, TransactionImpl and in depage logic. This should be put in one place.