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

[clebert.suconic@jboss.com]

"timfox" wrote : 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.
  | 

I had to use auto-format on this file, as it was messed up with spaces and tabs.

"timfox" wrote : 
  | 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.
  | 

Some of them were missing the final in a long time. I try to keep the finals but I had forgotten few of them.

anonymous wrote : 
  | 3) JournalImpl::disableautoreclaim - All attributes should be in the positive, not the negative as per discussion some time back.
  | 


The attribute is autoReclaim which is on positive.
disableAutoReclaim would be a method used by tests only, which is the same as setAutoReclaim(false), which I'm changing it to on TestableJournal.

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


Thanks for pointing out. I had forgotten that comment since the 6th commit of the file.

anonymous wrote : 
  | 
  | 5) writeTransaction - why do we need to write a "transaction complete" record with all the ids?
  | 
  | anonymous wrote : 
  |   | 
  |   | we record the number of elements per file. (The ID recorded is the FileSequenceID)
  |   | 
  |   | To make sure there is no holes on the commit of the transaction.
  |   | I record the number of elements per file used.
  |   | 
  |   | Say.. if a commit used 1000 elements on fileSequence=1, and 100 elements on fileSequence=2, I will record:
  |   | 
  |   | 1,1000,2,100
  |   | 
  |   | The first attempt was to record the total elements but a file could be reclaimed and part of the transaction gone,  so I couldn't verify the completeness of the commit.
  |   | 
  |   | 
  | 
  | 6) @supressWarnings - why do we need this annotation?
  | 

This conversion gives you a warning, even though is a legal conversion:
      Pair<Integer, Integer> values[] = (Pair<Integer, Integer>[]) new Pair[numberOfFiles];

I'm removing the annotation from that method, but I don't know how to solve the above warning.


anonymous wrote : 
  | 7) long load(LoadManager reloadManager) throws Exception - this method is only called internally so don't add it to the public interface
  | 
  | 

My intention is to keep it as part of the interface, and eventually replace the usage of load(List, List) by this method, where instead of allocating a lot of memory during load we send the messages directly to where they belong.

There are tests using that method also.

anonymous wrote : 
  | 8) JournalImpl::load:
  | if (recordType < ADD_RECORD || recordType > ROLLBACK_RECORD)
  |             {
  |                continue;
  |             }
  | Why do we ignore these?
  | 
  | 

This is part of the logic of looking for valid records on the file.

Any valid record will:
- start with a valid record descriptor byte (ADD_RECORD, UPDATE_RECORD.... ROLLBACK_RECORD).
- Have the fileSequenceID between the 2nd and 5th byte
- Have the checkSize at its 4 latest bytes.

I can't read records sequentially as we could have holes, and this will also couple with alignment from NIO<->AIO

anonymous wrote : 
  | 
  | 9) Please respect the correct sections of the file, public, private etc. e.g. debug() method in wrong section of the file
  | 
  | 

I did this on purpose.  When I'm debugging the file, I usually set trace=true manually and change the trace method. I'm keepting the static private method close to the attribute as it is easier that way.

anonymous wrote : 
  | 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.
  | 

Thanks! I was going to use this for callback errors, I solved it some other way and forgot the method there.

anonymous wrote : 
  | 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();
  | }
  | 

ok

anonymous wrote : 
  | 12) The locking on the append methods is effectively making the journal single threaded. Is this deliberate?
  | 

We aways had a record on appendRecord. We can't change currentFile until the checks on currentFile is done.
The new thing done was putting the appendRecord on a higher level, as the counters were being messed up. 
I can put the lock back on appendRecord, but I would to do more testings with that.

I would need to think about how to remove the lock at all. (There is a JIRA for that)

anonymous wrote : 
  | 13) Why addandGet(1) rather than incrementandGet().
  | 

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

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