On 10/24/11 8:40 PM, Selcuk AYA wrote:
On Mon, Oct 24, 2011 at 6:26 PM, Emmanuel Lecharny<[email protected]> wrote:
On 10/20/11 3:24 PM, Selcuk AYA wrote:
Continuing the previous incomplete email:)
Emmanuel wants to update the txns branch and I think it is a good time
to give a good status update. Following is what I have been doing and
the plan:
*Log Manager: A general log manager that can be used by txns as well.
Note that these logs are on disk. This is tested.
I reviewed the LogManager (and the associated classes) this week-end and
today. I committed a few fixes, no big errors. I have a few comments, not
all of them are mandatory, some of them are suggestions :
o Rename the Log class to TransactionLog, to avoid any confusion with the
ChangeLog. Same for LogManager, LogFileManager, LogFlushManager.
this log is not just for txns. İt is a general logging solution. In
fact you can make the changelog use it instead of storing eveything in
memory. Still if you have a better suggestion, I can change the names.
Ok, fine. I thought it wasn"'t generic, but looking at the code, you are
right. Actually, we already discussed with Julien Vermillard (MINA
project) about reusing this part in MINA.
o The methods in an Interface don't need to be declared as public : they
already are.
o Don't forget to apply the ADS formatter : 2 lines between each methods, a
newline before a return, etc…
OK. I think I will use the formatter to format it one final time
before check in after this time.
What you can do is to set Eclipse to format the code when you save the
files. It's incremental, you don't even have to think about it.
Same for headers, you can use the provided template that add them when
you create a new file. Very convenient.
o Don't add 'this.' when not necessary.
o Always use { and } when writing a if/for/while/do
OK
o Try to avoid inner classes, except to hide the inner class behavior
I sometimes use them to logically group data used together but
strongly tied to a class. and I think it makes sense to use them in
that way.
If they are really tied, yes, it makes sense to hide them. For instance,
the LogReader and writer are good candidate for inner classes : they
aren't likely to be used elswhere. But always be careful about creating
too many inner classes : it makes it difficult to find a class by its name.
o Don't declare an interface into another one
o LogFileManager should be moved to core-api
o LogScannerInterval should be moved to core-api
the two above are internal interfaces that are used internally. I
thought core-api has public interfaces.
Good point. This has to be discussed further.
o The LogFileRecords class might better be an interface ?
fine as it is I think. This just groups data related to internal
format of the log implementation. NOt every log implementation has to
have them.
Ok.
o fields in classes must be private or protected
OK
o Transaction should be moved to core-api
o TxnManagerInternal should be moved to core-api
See comment for LogFileManager and LogScannerInternal
Ok.
FTR, I have made some few modifications in the code, but nothing
structural so far. I just try to get a grip on what you've done, in
order to be able to help if anyone has some issue with this part later.
If you ask Alex, he will tell you that code cleaning is my favorite
game. It is. I *like* that. Although my perception of what is 'clean
code' can be quite personal, and I may be wrong from time to time.
Overall, the code you committed is pretty slick, and I liked it !
Thanks !
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
