directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Lécharny <elecha...@apache.org>
Subject Re: TXN work update
Date Mon, 24 Oct 2011 19:13:59 GMT
On 10/24/11 8:40 PM, Selcuk AYA wrote:
> On Mon, Oct 24, 2011 at 6:26 PM, Emmanuel Lecharny<elecharny@gmail.com>  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


Mime
View raw message