Julien Vermillard wrote:
Hi,
Hi Julien,
(I wish I didn't forgot anything)
and, ultimately :
- It should be easy to debug
- it should be FAST... :)
And memory friendly, cause we can have 1 chain per session.
most certainly ! But I don't think it's a major issue :)
<snip/>
2) Possible improvements
------------------------
The first big point is that we currently have one single chain, when
having two could help a lot. The current chain is two-ways, with
forward links and backwards link. As we have to handle incoming
requests but also write outgoing response, it has to be two-ways. But
one single chain is certainly not the best way to handle this. There
is no reason why the outgoing chain should be the same than the
incoming chain. We may even not use the same filters !
The decoder could be multi layered the the encoder not.
There is something missing in this sentence. Can you clarify ?
<snip/>
We should not have to traverse such a high number of methods in order
to process the filters. Not only this is time consuming (as we have
to invoke as many methods as we have lines in the stack trace, with
all the arguments to pass), but when it comes to debug filters, it's
an absolute nightmare. And we have injected ONE single filter in the
chain !
Debugging but reading the internal code too.
Very true ! Reading the actual code is, to be frank, a terrible
experience. Lack of documentation, lack of specification, lack of
comments, plus most certainly hacks which has been added to fulfill some
unknown requests (or may be to fix some performance issues which have
been spotted the last few years) make it almost impossible to convince
new comers to participate, due to the initial gap it costs. This is
exactly what the code should _not_ become in an Apache project, IMHO.
<snip/>
Last, not least, we are using a ChainBuilder to define the filter
chain. This structure is a fake FilterChain, which render the
manipulation tricky, and leads to confusion. I'm fine with the idea
of having a ChainBuilder, but it should be consistent. Another big
issue with this class is that it implements many useless methods
(like the ones which let you add or remove a filter giving its class,
or name, or the filter itself. One single method is enough : we
manipulate filters by their name...
I dislike this filter pick&placing API, I just teach how to use MINA
to another guy, this API confused him, and he created the chain in
reverse order and was unable to understand the bug.
Maybe we can make it more simple, and also write a better documentation.
It's really important to be able to add a new filter live. Now, for
those who build the chain in reverse order because they get confused, I
guess that if we have something which is easier to debug, the problem
will be solve quite quickly. FYI, I also have added some toString()
methods in the IoChainFilter class to expose the current chain as a list
of fiter's name, instead of a dummy list of [EMAIL PROTECTED] (sort of :).
<snip/>
So, wdyt, guys ?
Thanks !
Mostly agree here, I think we need to discuss the implementation in
detail,
Sure ! We can even split the initial mail in parts, and discuss all of
them. Even better, we can create a JIRA for each proposal, and follow
the discussion in those JIRAs.
another painful subject is sentMessage(..) reading the IoFilter
tutorial, explain all the braindamage it's doing.
:) I must say that, yep, you're right. I have put my feet in this water,
and it's icy... There is something badly wrong in this area, which need
to be fixed too. But I would keep it for a later discussion, as we can't
fix everything at the same time !
Perhaps we need to
kill this one with this refactoring, elsewhere you will need to do all
the "I preserve the initial message for triggering
IoHandler.sentMessage(..)" in your new code.
Can you elaborate a bit ? I'm not sure that those two problems overlap,
but I may miss something, too.
Thanks Julien !
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
