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#...
Reply to the post :
http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&a...