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