[jboss-dev-forums] [Design of Messaging on JBoss (Messaging/JBoss)] - Some feedback on latest journal changes

[timfox]

Clebert-

I've done a quick review of your latest changes, here are a few comments/questions:

1) I've made and committed several cosmetic changes - spacing, indentation etc.
2) final modifier was ommitted from many methods - I've added these and committed. Please remember to add these - they're very useful for catching bugs.
3) JournalImpl::disableautoreclaim - All attributes should be in the positive, not the negative as per discussion some time back.
4) JournalTransaction:commit, commented out line:
//throw new IllegalStateException("Cannot find add info " + n.b);
If the line is not needed any more, delete it, don't just comment it, or someone (like me) will wonder why it's still there
5) writeTransaction - why do we need to write a "transaction complete" record with all the ids?
6) @supressWarnings - why do we need this annotation?
7) long load(LoadManager reloadManager) throws Exception - this method is only called internally so don't add it to the public interface
8) JournalImpl::load:
if (recordType < ADD_RECORD || recordType > ROLLBACK_RECORD)
            {
               continue;
            }
Why do we ignore these?
9) Please respect the correct sections of the file, public, private etc. e.g. debug() method in wrong section of the file
10) JournalTransaction::get/isInvalid - I can't see this is actually used for anything, and if it is used it should be volatile or synchronized since could be called by different threads.
11) lock.acquire should go outside of try block, like this:
lock.acquire should go outside the try block, like this:
lock.acquire
try
{
...
}
finally
{
   lock.release();
}
12) The locking on the append methods is effectively making the journal single threaded. Is this deliberate?
13) Why addandGet(1) rather than incrementandGet().



View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4167851#4167851

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4167851