Hello Gazda, I promised I'll do a review but unfortunately I don't have the time so no use on waiting on me.
Cheers, 2012/6/29 Eric Charles <[email protected]>: > comments inline, Eric > > > On 06/28/2012 11:02 PM, Jochen Gazda wrote: >> >> Hi Eric, >> >> see inline. >> >> Best, >> >> gazda >> >> On Thu, Jun 28, 2012 at 4:57 PM, Eric Charles<[email protected]> wrote: >>> >>> On 06/28/2012 04:00 PM, Jochen Gazda wrote: >>>>> >>>>> >>>>> 1.- profile noTest on mailbox-integration-tester: ok >>>>> >>>>> 2.- LocalAndVirtualMailboxLocatorChain: what's the goal? >>>> >>>> >>>> >>>> MailDir is a file system based storage. There is the MaildirLocator >>>> interface for mapping of MailboxNames to file system directories and >>>> back. There are two basic MaildirLocator implementations: >>>> LocalSystemMaildirLocator (maps to /home/<user>/MailDir) and >>>> VirtualMailboxLocator (maps to<virtualRoot>/<domain>/<user>). The >>>> third one, LocalAndVirtualMailboxLocatorChain, cobines the two. >>>> >>> >>> Ok, got it. >>> (naming is just not consistent across LocalSystemMaildirLocator / >>> VirtualMailboxLocator / LocalAndVirtualMailboxLocatorChain (you have >>> Maildir >>> in the first, not in the others). >>> >>> >>> >>>>> 3.- MailboxPath is now MailboxName: Path sounded more like >>>>> folder/subfolder/subfolder. >>>> >>>> >>>> >>>> Yes, I agree, that is the case for common English. But there is no >>>> single occurence of 'path' in RFC3501. It speaks only about >>>> (hierarchical) names. On the other hand, MailboxPath/MailboxName is >>>> our internal term which does not map 1:1 to the IMAP hierachical name. >>>> We can name it however we want. I am not against going back to >>>> MailboxPath. >>>> >>> >>> The key is 'hierarchical'. I find MailboxPath correctly reflect this >>> point. >>> With MailboxName, you loose that, and MailboxHierarchicalName is just too >>> long. >>> >>> At first sight, I would prefer to revert to MailboxPath. >> >> >> There are several MailboxName* classes. I will have a look if renaming >> them all to MailboxPath* sounds plausible to me. >> >>>>> 4.- MailboxNameResolver on MailboxManager >>>> >>>> >>>> You mean that the methods from MailboxNameResolver should better move >>>> to MailboxManager? - Well, MailboxManager is defines storage >>>> operations, but MailboxNameResolver interprets names. That is >>>> something different. Two distinct MailboxNameResolvers are conceivable >>>> for a single MailboxManager; see (5) in my previous post ( >>>> /users/<username> vs. /users/<username>/INBOX ). >>>> >>> >>> I was just pointing an additional method MailboxNameResolver >>> getMailboxNameResolver(); on the MailboxManager interface. >>> >>> So good as is. >>> >>> >>>>> 5.- MailboxSession has no more PersonalSpace nor UserSpace but a >>>>> MailboxOwner. >>>> >>>> >>>> >>>> The capability to list namespace prefixes has moved from >>>> MailboxSession to MailboxNameResolver, which is now a member of >>>> MailboxSession. Listing namespace prefixes belongs to the mailbox >>>> hierarchy definition entailed in MailboxNameResolver. >>>> >>> >>> OK >>> >>> >>>> MailboxOwner is needed to distinguish groups from persons and 'normal' >>>> (domain-less) users from virtual users. >>>> >>> >>> So you assume a mailbox has always an owner. >>> Sounds logical, but is it sustained by the RFC? >> >> >> I have done it that way without thinking of RFC. It is quite practical >> to have it like that because one can set reasonable default ACLs which >> rely on the notion of owner. Dovecot and Cyrus do that. From >> http://wiki.dovecot.org/ACL : >> >>> The default ACL for mailboxes is to give the mailbox >>> owner all permissions and other users none. Mailboxes >>> in public namespaces don't have owners, so by default >>> no-one can access them. >> >> >> To have owners on mailboxes is surely not against the spirit of RFC >> 4314, which names owner in one of its examples: >> >>> For example, >>> in an implementation that uses UNIX mode bits, the rights "swite" are >>> tied, the "a" right is always granted to the owner of a mailbox and >>> is never granted to another user. >> >> >> It would probably be possible to do without mailbox owners but it >> seems really impractical to me. >> > > From your previous explanation, it sounds like MailboxOvwner is a good fit > for us (specifically for ACL and potentially for the rest). > > >>>>> 8. mmh, MailboxPath is still there. >>>> >>>> >>>> >>>> Yes, it is still there because there are still references to it from >>>> mailets project which I was not able to fix. >>>> Otherwise there are only refs in comments which can generally be >>>> replaced by MailboxName. >>>> >>> >>> Would it be solved if we revert from MailboxName to MailboxPath? >> >> >> Yes, perhaps. >> >>>>> 9. MailboxNameSerializer, MailboxNameBuilder, MailboxNameCodec, >>>>> MailboxNamespaceType, MailboxNameEscaper >>>> >>>> >>>> >>>> MailboxNameSerializer belongs to my first attempts. It could be made >>>> parent of MailboxNameCodec or replaced altogether. As for >>>> MailboxNameCodec cf. (6) of my previous post. >>>> MailboxNameBuilder - allows for saving some memory and string-copying >>>> when creating a new MailboxName. >>>> >>>> MailboxNameEscaper - ancillary interface for MailboxNameCodec. >>>> MailboxNameCodec implementations mostly differ only in the >>>> MailboxNameEscaper they use. >>>> >>> >>> I need to read back all this. >>> Is the goal of all these classes to better support mailbox name charsets >>> and >>> to build the hierachical mailbox name structure in a efficient way? >> >> >> No, currently it is not about mailbox name charsets, though >> MailboxNameCodec could serve also that purpose. >> MailboxNameCodec (and its usual member MailboxNameEscaper) are about >> parsing raw string names like "INBOX/asf" to segment collections like >> {"INBOX", "asf"} and serializing those collections back to strings. >> Their central task (when decoding) is to recognoze segment boundaries. >> For that purpose they define the delimiter (e.g. '/') that will be >> used for the given purpose. >> > > So MaiboxNameCodec has two responsibilities: > - Tokenize a 'Path' to an ordered list of 'PathSegment' (or 'PathToken') > - Build a 'Path' from an ordered list of 'PathSegment' (or 'PathToken') > > But we need to take car to not over-engineer, after all, PathSegment/Token > are just String. > > Still the fact is that it didn't popup to me reading the name > MailboxNameCodec. I also like the mantra 'one class, one responsibility', so > having two different classes coulb be an option (MailboxPathTokenizer and > MailboxPathBuilder). I am also fine leaving as such the MailboxNameCodec (or > MailboxPathCodec depending on your decision to revert on not to > MailboxPath). > > >>>>> 10. LikeSearchPatternEscaper: why deal with JCR, SQL in the API? >>>> >>>> >>>> >>>> It is not API, but common to many API consumers. Can you see a better >>>> place for it? >>>> >>> >>> I would prefer not to have them in the API, but we can leave it as such >>> for >>> now. >>> >>> >>>>> 11. More and more unit tests... :) >>>> >>>> >>>> >>>> Finite verb? >>>> >>> >>> ... is good :) >>> >>> >>>> >>>> Well, yes, definitely more typing - that is con. >>>> Pros are: >>>> - Handling mailbox names more reliably >>>> - Mailbox names are interpreted on one central place. >>>> - Namespace prefixes other than personal are now possible, esp. group >>>> folders are possible >>>> >>> >>> As con, you will end with a few more object to instantiate, mainly on >>> mailbox selection, but IMHO I this won't be an issue at all >>> >>> >>>>> To be honest, I didn't see well if and how the ACL are applied... :) >>>> >>>> >>>> >>>> New in my proposal is only that the ACLs can now be stored in JPA, JCR >>>> and HBase in addition to MailDir. They are still not enforced though. >>>> https://issues.apache.org/jira/browse/IMAP-358 is still open for that >>>> matter. >>>> >>> >>> OK, with these changes, you are now in a better place to implement the >>> ACL. >>> >>> >>>>> [1] >>>>> >>>>> >>>>> https://github.com/gazdahimself/current/commit/ae75a54400bc7aa93763657a48596d09ec357f98, >>>> >>>> >>>> >>>> [1] is now outdated, one can use >>>> https://github.com/gazdahimself/current/compare/master...MAILBOX-175 >>>> to see the relevant diffs. >>>> >>>> >>>>> >>>>> On 06/27/2012 06:55 PM, Jochen Gazda wrote: >>>>>> >>>>>> >>>>>> >>>>>> Gentlemen, >>>>>> >>>>>> I have finally managed it to publish my changeset on GitHub: >>>>>> https://github.com/gazdahimself/current/tree/MAILBOX-175 >>>>>> The state of my brach MAILBOX-175 is in sync with the currently latest >>>>>> svn revision 1354581. My brach MAILBOX-175 can also be diffed against >>>>>> svn revision 1354581 directly on GitHub. >>>>>> >>>>>> Sorry for the delay, I am new to git and I have a new job. >>>>>> >>>>>> Please comment. >>>>>> >>>>>> Best, >>>>>> >>>>>> gazda >>>>>> >>>>>> On Wed, Jun 13, 2012 at 1:51 PM, Ioan Eugen >>>>>> Stan<[email protected]> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> HI Gazda, >>>>>>> >>>>>>> Git is great. >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> 2012/6/13 Eric Charles<[email protected]>: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Gazda, >>>>>>>> >>>>>>>> I'm fine with the push to your personal github. It offers nice ui to >>>>>>>> show >>>>>>>> diffs. >>>>>>>> >>>>>>>> Thx, Eric >>>>>>>> >>>>>>>> >>>>>>>> On 06/13/2012 10:32 AM, Jochen Gazda wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Gentlemen, >>>>>>>>> >>>>>>>>> I could invest about 6 weeks of my time into solving >>>>>>>>> https://issues.apache.org/jira/browse/MAILBOX-175 and >>>>>>>>> https://issues.apache.org/jira/browse/MAILBOX-167. >>>>>>>>> The result is quite a huge and deep changeset. There is a de facto >>>>>>>>> replacement for MailboxPath - so you can imagine, how many classes >>>>>>>>> were changed. >>>>>>>>> >>>>>>>>> Before going too much into details I wanted to agree on a way how >>>>>>>>> my >>>>>>>>> changeset could find its way into SVN. >>>>>>>>> >>>>>>>>> The changes should be discussed before they are committed to trunk. >>>>>>>>> But I am not sure what is the best way to share my changes before >>>>>>>>> they >>>>>>>>> are committed to trunk. >>>>>>>>> Would cloning from http://git.apache.org/ and pushing changes to my >>>>>>>>> personal GitHub repo be a viable solution? >>>>>>>>> I am also ready to send my zipped workspace (~30MB) to anybody who >>>>>>>>> wants to have a quick look. >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> >>>>>>>>> gazda >>>>>>>>> >>>>>>>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> To unsubscribe, e-mail: [email protected] >>>>>>>>> For additional commands, e-mail: [email protected] >>>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> eric | http://about.echarles.net | @echarles >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> To unsubscribe, e-mail: [email protected] >>>>>>>> For additional commands, e-mail: [email protected] >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Ioan Eugen Stan / http://axemblr.com / Tools for Clouds >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> To unsubscribe, e-mail: [email protected] >>>>>>> For additional commands, e-mail: [email protected] >>>>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: [email protected] >>>>>> For additional commands, e-mail: [email protected] >>>>>> >>>>> >>>>> -- >>>>> eric | http://about.echarles.net | @echarles >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: [email protected] >>>>> For additional commands, e-mail: [email protected] >>>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: [email protected] >>>> For additional commands, e-mail: [email protected] >>>> >>> >>> -- >>> eric | http://about.echarles.net | @echarles >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: [email protected] >>> For additional commands, e-mail: [email protected] >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> > > -- > eric | http://about.echarles.net | @echarles > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > -- Ioan Eugen Stan / CTO / http://axemblr.com --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
