Hi, Quick fact: you can also do review with inline comments on Github by clicking on the plus icon on the left while viewing the commit.
I think comments are more valuable there because you can put them next to the code and add context to the words. This is old, but I just found out and figured it might help. Cheers, 2012/7/10 Ioan Eugen Stan <stan.ieu...@gmail.com>: > 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 <e...@apache.org>: >> 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<e...@apache.org> 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<stan.ieu...@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> HI Gazda, >>>>>>>> >>>>>>>> Git is great. >>>>>>>> >>>>>>>> Cheers, >>>>>>>> >>>>>>>> 2012/6/13 Eric Charles<e...@apache.org>: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> 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: server-dev-unsubscr...@james.apache.org >>>>>>>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> eric | http://about.echarles.net | @echarles >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>>>>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Ioan Eugen Stan / http://axemblr.com / Tools for Clouds >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>>>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>>>>> >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>>>> >>>>>> >>>>>> -- >>>>>> eric | http://about.echarles.net | @echarles >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>>> >>>> >>>> -- >>>> eric | http://about.echarles.net | @echarles >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>>> For additional commands, e-mail: server-dev-h...@james.apache.org >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >>> For additional commands, e-mail: server-dev-h...@james.apache.org >>> >> >> -- >> eric | http://about.echarles.net | @echarles >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org >> For additional commands, e-mail: server-dev-h...@james.apache.org >> > > > > -- > Ioan Eugen Stan / CTO / http://axemblr.com -- Ioan Eugen Stan / CTO / http://axemblr.com --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org