On Fri, Mar 30, 2012 at 10:58 PM, Emmanuel Lécharny <elecharny@gmail.com> wrote:
Hi guys,

as I'm goig to be offline often this week-end, just a short mail to give you some info about what I'm doing and how far I currently am.

The idea is to get rid of the OneLevel index, as I already said in one of my previous mails. I had hard time to understand how to use Cursors, especially on top of the RdnIndex, but as of today, I finally get something working fine to replace the OneLevel index.
I'm removing all the calls to this index in the code one by one, checking that teh server still works as expected. I still have to deal with some calls in the LdifPartition, but I'm close.

We can now use the RdnIdx to get a list of children for a given entry, and I have designed a dedicated cursor (ChildrenCursor) for that. Right now, it's more a hack than anything else (I'm counting the nulber of children while returning them, stopping when I reach the expected number of children), but I will use a better solution later (basically, I'll check the parent ID of each element I pull from the index). It works fine.


Cool glad to see this working well. 
 
At the same time, I'm trying to cleanup a bit the Cursor hierarchy. I was able to remove a couple of classes and interfaces that were useless, and I'm pretty sure we can go farther. The generics are a bit messy, and we often have to mask the to get things working.


I think it's a big mistake to couple the new feature you're adding with cleanup and refactoring:

(1) This 'cleanup' effort convolutes evaluating your branch come time to merge. I'm sure you want people to review your work but if you mix this up with another aspect, 'the cleanup' effort, then this creates extra background noise. Just add this feature so we can clearly see what the feature changes are when it's time to do an evaluation. I know you naturally will want us to collaborate when you're ready to merge. Keeping the refactoring down to a minimum while adding the feature will make it easier and less time consuming for us to review these changes and provide feedback.

(2) You yourself say you're new to the code in this region of the server. Learning the code by changing it is not a good practice. If you want to do cleanups do them at another time, or before you start the feature work, or just note them, or do so in a separate branch. Either way separate these two initiatives.

(3) This is a very complex region of code and a lot of thought has gone into it. I am sure many things can be improved but they should be done with care. Anyone of us can think something is useless but really it might not be, but we don't see this until later. So there's less risk to this if you separate the cleanup effort from the removal of the one and sub level indices.

(4) Changing the code so you feel more comfort while working in it is understandable but this can impact others. It's good to cleanup but your changes might cause problems for others. You should be extra careful especially because this is complex code, something that has been around for ages, and you're new to it.

Please don't take this as an attack but as a recommendation from a peer. My concern is that I'm going to look at this code down the line and see that many things were changed and those changes made it unfamiliar to me without much gain. I'm sure you also want me to be able to work with you in that code base so we can bounce ideas around together. If the code is reworked without good reasons this makes it that much harder for me and others to participate. Plus I can no longer give the same amount of time I gave in the past so I want to help but if this takes very long I cannot do so as well as I'm asked to .... just trying to balance all this.

I'm also quite sure that we should abstract more on top of the Table implementation : we don't have a generic Browser, and that leads to a duplication of cursors (Avl cursors and Jdbm cursors). We most certainly can do better.


Again I wish you could separate these various initiatives so it makes it very clear in the differentials. Mixing all these objectives in one big branch merge will make getting others to work together with you more difficult.
 
I will continue up to the point I can completely remove the OneLevel index (which is still created and manager), then I'll do the same thing for the SubLevelIndex.


Can't wait to see both indices go. This is a good objective and I'm thankful someone is taking this on. 
 
Btw, that could help the txn layer, as there will be two less index to manage...


+1
 
That's pretty much it for this week.  Have fun !


You too have a good weekend.


--
Best Regards,
-- Alex