You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by Stefano Bagnara <ap...@bago.org> on 2008/09/15 15:04:36 UTC
[imap] reviewing dependencies (packages / modules)
Hi Robert,
I just did some review of the resulting imap product and I'm impressed.
While I reviewed I created a graph for the current modules dependencies:
http://people.apache.org/~bago/imap/james-imap-module-dependencies.gif
Maybe this graph could be included in the website once we'll have one.
I have some question/comment:
1) Why do we have STATUSResponse and StatusResponse ?
2) There are 2 package cycles:
imap.message.request.base <-> imap.message.request.imap4rev1
mailboxmanager.impl <-> mailboxmanager.util
I'd like them to be removed:
2a) The imap.message.request could be mergerd into a single package (in
base there is an imap4rev1 class!).
2b) The mailboxmanage cycle is because of UidChangeTracker. Moving to to
"impl" should fix this too.
3) I see some module is really small (seda) while other modules are
really big (codec). I graphed the package dependency tree (it is a tree
only after grouping the 2 cycles above) and it resulted that the codec
module have 2 separated hierarchies:
http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
Does it worth splitting? (note that imap.message.request.base+imap4rev1
and imap.command.imap4rev1 can be moved in both "groups").
3b) maybe we could even shorten package names: codec.encode => encode
and codec.decode => decode
3c) another option would be to extract the imap.message package to its
own module (message). codec (or both decode/encode if it is splitted)
would then depend on this message module). If I'm not wrong processor
would so depend only on "message" and not on codec(decode/encode).
4) What about removing the "experimental" string from seda packages? I
think the "0.1" release version makes it already "experimental".
You did a great job, indeed,
Thank you,
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Tue, Sep 16, 2008 at 8:25 PM, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
>>
>> On Tue, Sep 16, 2008 at 12:20 AM, Stefano Bagnara <ap...@bago.org> wrote:
>>>
>>> Robert Burrell Donkin ha scritto:
>>>>
>>>> On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <ap...@bago.org>
<snip>
>>>>> 3c) another option would be to extract the imap.message package to its
>>>>> own
>>>>> module (message). codec (or both decode/encode if it is splitted) would
>>>>> then
>>>>> depend on this message module). If I'm not wrong processor would so
>>>>> depend
>>>>> only on "message" and not on codec(decode/encode).
>>>>
>>>> a separate message module makes sense
>>>>
>>>> this area is a little messy and hope it can be resolved more elegantly
>>>> once some of the deprecated structure's removed.
>>>
>>> I created a message module but I also moved there the "encode" part of
>>> the
>>> codec.
>>> I did this temporarily because I don't know how you want to deal with the
>>> ant api-library-function-deployment layering and the above separation
>>> allowed me to not introduce another layer.
>>
>> i'll take a look at this once you've finished
>>
>>> How would you handle this in the self imposed layer rule?
>>
>> no real need to keep the rule for IMAP
>>
>> when decomposing a project as large and coupled as james, the tendency
>> is to create a mess of modules with deep and complex interdependencies
>> rather than working to correctly separate concerns. the
>> api/library/function split (plus deployment) prevents this from
>> happening. in order to fit the modules into this constraint it's
>> necessary to separate concerns and adopt a relatively coursely grained
>> and simple system of coupling.
>
> So, how do you propose me to solve the "message" module issue?
>
> A) move .message. to api?
> B) create a new layer (between api and libraries) for .message. ?
> C) leave as is (message+encode in a module, decode in the other module).
> D) revert (merge again message and codec).
>
> I'll give a try to the api module to understand if some class can be moved
> from api to message in order to make the 2 modules indipendend, but I doubt
> this is possible.
leave as is for now
>>> Maybe the whole "message" package should be moved to imap-api instead?
>>> Looking again at it I don't fully understand why some of "imap.message"
>>> should be in api and something else should be in codec (now message)
>>> library. Can you give me any hint?
>>
>> i was planning to review this post M1 so it's probably that stuff's
>> just badly organised
>
> I find the current "message"+"codec" module better than before (even if
> encode is in message module.
> Maybe we should simply rename "codec" to "decoder" and release M1.
> (I say this because I understand you want to do M1 early..)
ok
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On Tue, Sep 16, 2008 at 12:20 AM, Stefano Bagnara <ap...@bago.org> wrote:
>> Robert Burrell Donkin ha scritto:
>>> On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <ap...@bago.org> wrote:
>>>> (note that imap.message.request.base+imap4rev1 and
>>>> imap.command.imap4rev1 can be moved in both "groups").
>>>> 3b) maybe we could even shorten package names: codec.encode => encode and
>>>> codec.decode => decode
>>> +1
>>>
>>> probably want to shorten imapserver -> imap as well
>> I thought you decided for imap when the code was not client/server specific
>> and imapserver when the code was for the server-side, but if this is not the
>> case maybe imap is better.
>
> going forward, i think it'd be best for james server to use imapserver
> and the component to use imap
ok
>>> i'm not sure whether it's worthwhile including 'imap4rev1' in the
>>> package since it's the only revision in common use
>> I think it should be removed. I don't like too much nested package trees.
>
> please dive in
ok
>>>> 3c) another option would be to extract the imap.message package to its
>>>> own
>>>> module (message). codec (or both decode/encode if it is splitted) would
>>>> then
>>>> depend on this message module). If I'm not wrong processor would so
>>>> depend
>>>> only on "message" and not on codec(decode/encode).
>>> a separate message module makes sense
>>>
>>> this area is a little messy and hope it can be resolved more elegantly
>>> once some of the deprecated structure's removed.
>> I created a message module but I also moved there the "encode" part of the
>> codec.
>> I did this temporarily because I don't know how you want to deal with the
>> ant api-library-function-deployment layering and the above separation
>> allowed me to not introduce another layer.
>
> i'll take a look at this once you've finished
>
>> How would you handle this in the self imposed layer rule?
>
> no real need to keep the rule for IMAP
>
> when decomposing a project as large and coupled as james, the tendency
> is to create a mess of modules with deep and complex interdependencies
> rather than working to correctly separate concerns. the
> api/library/function split (plus deployment) prevents this from
> happening. in order to fit the modules into this constraint it's
> necessary to separate concerns and adopt a relatively coursely grained
> and simple system of coupling.
So, how do you propose me to solve the "message" module issue?
A) move .message. to api?
B) create a new layer (between api and libraries) for .message. ?
C) leave as is (message+encode in a module, decode in the other module).
D) revert (merge again message and codec).
I'll give a try to the api module to understand if some class can be
moved from api to message in order to make the 2 modules indipendend,
but I doubt this is possible.
>> Maybe the whole "message" package should be moved to imap-api instead?
>> Looking again at it I don't fully understand why some of "imap.message"
>> should be in api and something else should be in codec (now message)
>> library. Can you give me any hint?
>
> i was planning to review this post M1 so it's probably that stuff's
> just badly organised
I find the current "message"+"codec" module better than before (even if
encode is in message module.
Maybe we should simply rename "codec" to "decoder" and release M1.
(I say this because I understand you want to do M1 early..)
>>>> 4) What about removing the "experimental" string from seda packages? I
>>>> think
>>>> the "0.1" release version makes it already "experimental".
>>> +1
>> I opened a JIRA assigned to me for this, will do another day.
>
> great
>
> thanks for the hard work :-)
You did most of it :-)
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Tue, Sep 16, 2008 at 12:20 AM, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
>>
>> On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <ap...@bago.org> wrote:
>>>
>>> Hi Robert,
>>>
>>> I just did some review of the resulting imap product and I'm impressed.
>>
>> it's still a little loose: the commons-lang dependency isn't
>> absolutely required by anything other than torque
>
> org.apache.james.imapserver.codec.encode.EncoderUtils only includes a
> encodeDateTime method and it uses
> org.apache.commons.lang.time.FastDateFormat.
yes - it just requires someone sitting down and writing a suitable
data parser. not a big job but i wanted to freeze the code until an M1
was cut.
>>> I have some question/comment:
>>>
>>> 1) Why do we have STATUSResponse and StatusResponse ?
>>
>> IMAP specification
>>
>> i've attempt to name objects from the specification but the
>> specification re-uses the term 'status'. there's quite a lot of
>> complexity which could usefully be pruned so hopefully one of these
>> can be eliminated.
>
> My issue was mainly about the ALL-CAPS choice.
packaging might have been confusing but M1 is only intended to
represent an intermediary phase of the code
>>> 3) I see some module is really small (seda) while other modules are
>>> really
>>> big (codec). I graphed the package dependency tree (it is a tree only
>>> after
>>> grouping the 2 cycles above) and it resulted that the codec module have 2
>>> separated hierarchies:
>>>
>>> http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
>>> Does it worth splitting?
>>
>> possibly
>>
>> again, this is something i'd planned to address post-M1. M1 was
>> intended just to be a working IMAP for james, not anything more. the
>> code structure could do with revision.
>
> Sure! I simply wanted to get more from my review process :-)
> I'm not interested in imap ATM, but I find that if I keep reviewing diffs I
> slowly loose the understanding of what we have and I may no more be in-topic
> when I discuss or I cast my votes. I also wanted to see how you dealt with
> socket testing without avalon.
note that IMAP doesn't do real socket testing
it might be worthwhile setting up some integration smoke tests which
run against a full packaged default james server deployed for test.
this would allow basic testing of deployed sockets.
>>> (note that imap.message.request.base+imap4rev1 and
>>> imap.command.imap4rev1 can be moved in both "groups").
>>> 3b) maybe we could even shorten package names: codec.encode => encode and
>>> codec.decode => decode
>>
>> +1
>>
>> probably want to shorten imapserver -> imap as well
>
> I thought you decided for imap when the code was not client/server specific
> and imapserver when the code was for the server-side, but if this is not the
> case maybe imap is better.
going forward, i think it'd be best for james server to use imapserver
and the component to use imap
>> i'm not sure whether it's worthwhile including 'imap4rev1' in the
>> package since it's the only revision in common use
>
> I think it should be removed. I don't like too much nested package trees.
please dive in
>>> 3c) another option would be to extract the imap.message package to its
>>> own
>>> module (message). codec (or both decode/encode if it is splitted) would
>>> then
>>> depend on this message module). If I'm not wrong processor would so
>>> depend
>>> only on "message" and not on codec(decode/encode).
>>
>> a separate message module makes sense
>>
>> this area is a little messy and hope it can be resolved more elegantly
>> once some of the deprecated structure's removed.
>
> I created a message module but I also moved there the "encode" part of the
> codec.
> I did this temporarily because I don't know how you want to deal with the
> ant api-library-function-deployment layering and the above separation
> allowed me to not introduce another layer.
i'll take a look at this once you've finished
> How would you handle this in the self imposed layer rule?
no real need to keep the rule for IMAP
when decomposing a project as large and coupled as james, the tendency
is to create a mess of modules with deep and complex interdependencies
rather than working to correctly separate concerns. the
api/library/function split (plus deployment) prevents this from
happening. in order to fit the modules into this constraint it's
necessary to separate concerns and adopt a relatively coursely grained
and simple system of coupling.
> Maybe the whole "message" package should be moved to imap-api instead?
> Looking again at it I don't fully understand why some of "imap.message"
> should be in api and something else should be in codec (now message)
> library. Can you give me any hint?
i was planning to review this post M1 so it's probably that stuff's
just badly organised
>>> 4) What about removing the "experimental" string from seda packages? I
>>> think
>>> the "0.1" release version makes it already "experimental".
>>
>> +1
>
> I opened a JIRA assigned to me for this, will do another day.
great
thanks for the hard work :-)
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <ap...@bago.org> wrote:
>> Hi Robert,
>>
>> I just did some review of the resulting imap product and I'm impressed.
>
> it's still a little loose: the commons-lang dependency isn't
> absolutely required by anything other than torque
org.apache.james.imapserver.codec.encode.EncoderUtils only includes a
encodeDateTime method and it uses
org.apache.commons.lang.time.FastDateFormat.
Otherwise it is only used by tests or by torque.
>> While I reviewed I created a graph for the current modules dependencies:
>> http://people.apache.org/~bago/imap/james-imap-module-dependencies.gif
>>
>> Maybe this graph could be included in the website once we'll have one.
>
> +1
>
> they are really cool :-)
Thanks!
>> I have some question/comment:
>>
>> 1) Why do we have STATUSResponse and StatusResponse ?
>
> IMAP specification
>
> i've attempt to name objects from the specification but the
> specification re-uses the term 'status'. there's quite a lot of
> complexity which could usefully be pruned so hopefully one of these
> can be eliminated.
My issue was mainly about the ALL-CAPS choice.
>> 2) There are 2 package cycles:
>> imap.message.request.base <-> imap.message.request.imap4rev1
>> mailboxmanager.impl <-> mailboxmanager.util
>
> as few as that?
yes, it seems :-)
> this is an issue that i'd intended to come back and address post-M1
>
>> I'd like them to be removed:
>> 2a) The imap.message.request could be mergerd into a single package (in base
>> there is an imap4rev1 class!).
>> 2b) The mailboxmanage cycle is because of UidChangeTracker. Moving to to
>> "impl" should fix this too.
>
> +1
>
> dive right in
Done.
>> 3) I see some module is really small (seda) while other modules are really
>> big (codec). I graphed the package dependency tree (it is a tree only after
>> grouping the 2 cycles above) and it resulted that the codec module have 2
>> separated hierarchies:
>> http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
>> Does it worth splitting?
>
> possibly
>
> again, this is something i'd planned to address post-M1. M1 was
> intended just to be a working IMAP for james, not anything more. the
> code structure could do with revision.
Sure! I simply wanted to get more from my review process :-)
I'm not interested in imap ATM, but I find that if I keep reviewing
diffs I slowly loose the understanding of what we have and I may no more
be in-topic when I discuss or I cast my votes. I also wanted to see how
you dealt with socket testing without avalon.
>> (note that imap.message.request.base+imap4rev1 and
>> imap.command.imap4rev1 can be moved in both "groups").
>> 3b) maybe we could even shorten package names: codec.encode => encode and
>> codec.decode => decode
>
> +1
>
> probably want to shorten imapserver -> imap as well
I thought you decided for imap when the code was not client/server
specific and imapserver when the code was for the server-side, but if
this is not the case maybe imap is better.
> i'm not sure whether it's worthwhile including 'imap4rev1' in the
> package since it's the only revision in common use
I think it should be removed. I don't like too much nested package trees.
>> 3c) another option would be to extract the imap.message package to its own
>> module (message). codec (or both decode/encode if it is splitted) would then
>> depend on this message module). If I'm not wrong processor would so depend
>> only on "message" and not on codec(decode/encode).
>
> a separate message module makes sense
>
> this area is a little messy and hope it can be resolved more elegantly
> once some of the deprecated structure's removed.
I created a message module but I also moved there the "encode" part of
the codec.
I did this temporarily because I don't know how you want to deal with
the ant api-library-function-deployment layering and the above
separation allowed me to not introduce another layer.
How would you handle this in the self imposed layer rule?
Maybe the whole "message" package should be moved to imap-api instead?
Looking again at it I don't fully understand why some of "imap.message"
should be in api and something else should be in codec (now message)
library. Can you give me any hint?
>> 4) What about removing the "experimental" string from seda packages? I think
>> the "0.1" release version makes it already "experimental".
>
> +1
I opened a JIRA assigned to me for this, will do another day.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <ap...@bago.org> wrote:
> Hi Robert,
>
> I just did some review of the resulting imap product and I'm impressed.
it's still a little loose: the commons-lang dependency isn't
absolutely required by anything other than torque
> While I reviewed I created a graph for the current modules dependencies:
> http://people.apache.org/~bago/imap/james-imap-module-dependencies.gif
>
> Maybe this graph could be included in the website once we'll have one.
+1
they are really cool :-)
> I have some question/comment:
>
> 1) Why do we have STATUSResponse and StatusResponse ?
IMAP specification
i've attempt to name objects from the specification but the
specification re-uses the term 'status'. there's quite a lot of
complexity which could usefully be pruned so hopefully one of these
can be eliminated.
> 2) There are 2 package cycles:
> imap.message.request.base <-> imap.message.request.imap4rev1
> mailboxmanager.impl <-> mailboxmanager.util
as few as that?
this is an issue that i'd intended to come back and address post-M1
> I'd like them to be removed:
> 2a) The imap.message.request could be mergerd into a single package (in base
> there is an imap4rev1 class!).
> 2b) The mailboxmanage cycle is because of UidChangeTracker. Moving to to
> "impl" should fix this too.
+1
dive right in
> 3) I see some module is really small (seda) while other modules are really
> big (codec). I graphed the package dependency tree (it is a tree only after
> grouping the 2 cycles above) and it resulted that the codec module have 2
> separated hierarchies:
> http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
> Does it worth splitting?
possibly
again, this is something i'd planned to address post-M1. M1 was
intended just to be a working IMAP for james, not anything more. the
code structure could do with revision.
> (note that imap.message.request.base+imap4rev1 and
> imap.command.imap4rev1 can be moved in both "groups").
> 3b) maybe we could even shorten package names: codec.encode => encode and
> codec.decode => decode
+1
probably want to shorten imapserver -> imap as well
i'm not sure whether it's worthwhile including 'imap4rev1' in the
package since it's the only revision in common use
> 3c) another option would be to extract the imap.message package to its own
> module (message). codec (or both decode/encode if it is splitted) would then
> depend on this message module). If I'm not wrong processor would so depend
> only on "message" and not on codec(decode/encode).
a separate message module makes sense
this area is a little messy and hope it can be resolved more elegantly
once some of the deprecated structure's removed.
> 4) What about removing the "experimental" string from seda packages? I think
> the "0.1" release version makes it already "experimental".
+1
feel free to dive in and patch your proposed changes in
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [imap] reviewing dependencies (packages / modules)
Posted by Stefano Bagnara <ap...@bago.org>.
Stefano Bagnara ha scritto:
> [...]
> 3) I see some module is really small (seda) while other modules are
> really big (codec). I graphed the package dependency tree (it is a tree
> only after grouping the 2 cycles above) and it resulted that the codec
> module have 2 separated hierarchies:
> http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
>
> Does it worth splitting? (note that imap.message.request.base+imap4rev1
> and imap.command.imap4rev1 can be moved in both "groups").
> 3b) maybe we could even shorten package names: codec.encode => encode
> and codec.decode => decode
> 3c) another option would be to extract the imap.message package to its
> own module (message). codec (or both decode/encode if it is splitted)
> would then depend on this message module). If I'm not wrong processor
> would so depend only on "message" and not on codec(decode/encode).
I forgot the graph for 3c:
http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque-message.gif
I have to add that packages are already very good for the codec module,
so the split in modules is not a requirement. Maybe extracting "message"
is more important than separating encode/decode in codec because it
results in reduced dependencies for the processor.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org