[index] OneLevel index removal headsup

[Emmanuel Lécharny <el...@gmail.com>]

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.

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

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.

Btw, that could help the txn layer, as there will be two less index to 
manage...

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

-- 
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

Re: [index] OneLevel index removal headsup

[Alex Karasulu <ak...@apache.org>]

On Fri, Mar 30, 2012 at 10:58 PM, Emmanuel Lécharny <el...@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