Paging code review

[timfox]

As 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.

[clebert.suconic]

A lot of the minor tasks are done already...

I have just a point regarding the corrupt pages. I'm going to measure if we need concurrent writes (they were written for AIO), and if they are, we still need to deal with holes (even on NIO).

Only thing is I need to add a counter per page, not just a global counter any more, same way we do on the journal.

But if we disable concurrent writes this is moot. Let me check it first.

[timfox]

One other point:

11) Need to sync paging on non transactional persistent page, if corresponding config property is set

[timfox]

12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).

[clebert.suconic]

just to register this somewhere..

For 12) We talked about using org.jboss.messaging.util.Base64 for the directory name.

[clebert.suconic]

1. Use OrdereredExecutorFactory for thread management. This should make the global pager redundant.

I have this done on my local copy already.. I'm just waiting some tests to finish before I commit this tomorrow.

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.

done

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?

The readWriteLock is used to avoid synchronized access on the verification on isPaging(). I wanted to not create any contention when paging is not activated.

I have changed the code not allow multiple writes, but the lock schema is pretty much the same.

6. PagingStoreImpl::page()

Calculation of block size and lock acquisition:

done

7. PagingStoreImpl::page

pagedUsedSize is being incremented twice?

Not really.. openNewPage is setting it to 0, then it needs to be added again.

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

I want to do that as part HugeTransaction task.

10. Currently routing code is duplicated in ServerSessionImpl::send, TransactionImpl and in depage logic. This should be put in one place.

This goes beyond of paging... but I will do that tomorrow.


I'm still missing 2, but that should be easy. (Unless we decide to do something more complex such as hole-detection the way I have done on the Journal).

[timfox]

What about 5) ?

[clebert.suconic]

5) The tricky part on paging is to let depage happen at the same time messages are being paged.

Depage will aways return old pages, but when depageRate>pageRate, we will eventually be depaging the currentPage, on which case I open a newPage and return the current one.

But if no messages were produced on that new page, it will be empty. When we return that empty page, we will exit the pageMode and from that point we will start using the journal again.

This is done this way because I can't lock page while depaging records. It all happens concurrently. Once an address enters in pageMode, it will be in pageMode until the rate of removing messages on page is higher the rate on adding messages from page.

[clebert.suconic]

The only missing parts now are:

13) Add a limit number of Threads limit on the Main Executor for depage. (Which is a very minor task)

8-9) it will be made as part of another task

and


10) Which is not directly related to Paging itself.

[timfox]

In your latest commit flow control appears broken, and not implemented as we discussed.

This needs to be discussed later

[clebert.suconic]

My idea yesterday was to use the Header Size as the last chunk size on flow control, but in a second thought yes this is broken as I could have messages never arriving as the very first flow control was not confirmed.

I'm fixing it.

I will add a property on ClientMessage to store the next flowControl to be used. On regular message that will aways be the encodeSize, and on LargeMessages the size of the last chunk.

[clebert.suconic]

10) I have added three methods on PostOffice for this:

 /** Deliver references previously routed */
 void deliver(List<MessageReference> references);

 void scheduleReferences(long scheduledDeliveryTime, List<MessageReference> references) throws Exception;

 void scheduleReferences( long transactionID, long scheduledDeliveryTime, List<MessageReference> references) throws Exception;

I think the Postoffice would be the right place, since this is related to what needs to be done post-routing, during commit.. depage.. etc. If that's not the right place it should be an easy move though.


- I have also fixed ScheduledDeliveries on Paging, and few other issues with sizing and Expiry on Paging.

[timfox]

"timfox" wrote:
12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).

Is this done yet? (Doesn't look like it).

If not, please make sure it is tracked in a JIRA.

Actually, can you look through all the paging and large message threads and make sure everything that is not complete yet is tracked properly. It seems we have some loose ends here.....

[timfox]

"timfox" wrote:

"timfox" wrote:
12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).

Is this done yet? (Doesn't look like it).

Ah sorry. I can see it is done.

One thing:

final String destinationDirectory = directory + "/" + Base64.encodeBytes(destinationName.getData(), Base64.URL_SAFE);

Is it same to assume "/" is file separator on all OSes?

Should we use Systm.getproperty("file.separator") ?

[clebert.suconic]

I have used '/' in both Windows and Linux, and it has aways worked.


We can change that if you prefer.