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().

[timfox]

One more:

JournalTransaction numberOfElements - does the AtomicInteger ever get incremented?

[clebert.suconic]

"timfox" wrote:
One more:

JournalTransaction numberOfElements - does the AtomicInteger ever get incremented?

private AtomicInteger getCounter(final JournalFile file)
 {
 AtomicInteger value = numberOfElements.get(file.getOrderingID());

 if (value == null)
 {
 value = new AtomicInteger();
 numberOfElements.put(file.getOrderingID(), value);
 }

 return value;
 }

[clebert.suconic]

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

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.

5) writeTransaction - why do we need to write a "transaction complete" record with all the ids?

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.

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.

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

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.

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.

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

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

We aways had a lock 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)

13) Why addandGet(1) rather than incrementandGet().

I've changed it. Thanks!

[timfox]

The first attempt was to record the total elements but a file could be reclaimed and part of the transaction gone,

How can a file be reclaimed if it it contains transactional records that haven't been committed or rolled back? We should never allow *part* of a transaction to disappear.

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.

I was referring to the debug() method not the static trace method

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)

Yes, we need to revisit the concurrency.

[clebert.suconic]

"timfox" wrote:

The first attempt was to record the total elements but a file could be reclaimed and part of the transaction gone,

How can a file be reclaimed if it it contains transactional records that haven't been committed or rolled back? We should never allow *part* of a transaction to disappear.

I didn't change anything on reclaiming. Reclaiming is working the same way as aways.

This is jut how I validate if the transaction has all the records it was supposed to have.

When you are reloading a transaction, you need to verify you have the exact same number of records as before. If the transactional was legally committed part of it could have been legally reclaimed and that doesn't mean the transaction is unhealthy.

If we didn't allow part of a committed transaction be reclaimed, reclaiming would never delete any files if someone uses transactions. (a transaction could be spread between multiple files).

"Tim Fox" wrote:

I was referring to the debug() method not the static trace method

That method is public, and part of TestableJournal. It prints the Reclaiming Status on a StringBuffer. I use it every time I investigate a bug on the journal, on testcases.

[timfox]

If we didn't allow part of a committed transaction be reclaimed, reclaiming would never delete any files if someone uses transactions. (a transaction could be spread between multiple files).

I understood you were referring to uncommitted transactions. For committed then sure they can be reclaimed.

That method is public, and part of TestableJournal. It prints the Reclaiming Status on a StringBuffer. I use it every time I investigate a bug on the journal, on testcases.

I've nothing against that method per se, just saying it's in the wrong place.
It's currently in the section of the file reserved for method on the Journal interface, but it should be in the TestableJournal implementation section. That was my point, just respect the areas of the file, no biggie!