Ok so its seems like you like the proposal_v3 so far (just minor naming changes etc). I will commit it then and try to improve the javadocs a bit etc.. Thx for the review..
Bye, Norman 2010/8/20 Eric Charles <e...@apache.org>: > Hi All, > Comments inside. > Tks, > Eric > > On 08/20/2010 04:53 PM, Tim-Christian Mundt wrote: >> >> Hi guys, >> >> I began looking into the API stuff. I'd like to simplify the Mapper >> hierarchy, which however will have no direct impact on the API. Will >> upload a patch tomorrow. >> > Will look at it :) >> >> I agree, we should name the interface MessageManager, that's what I >> thought. > > +1 >> >> The "proposal_v3" is reasonable, I think. To make it clearer, we should >> maybe call "AbstractStoreMessageManager" "StoreMessageManager" and >> qualify "StoreMessageManager" to "MapperStoreMessageManager". I think, >> that's what Eric also proposed. >> > Sounds good > (loosely proposal : MappingStoreMessageManager instead of > MapperStoreMessageManager :) >> >> Concerning composition: I think the composition part in the Store stuff >> is the mappers which get plugged into the Managers. The only reason so >> far for extending Store*Manager is the creation of certain objects. >> Nonetheless, you are right, Eric, there are quite extensive hierarchies >> in James and maybe we can simplify them. However, inheritance here is >> not used to change behavior but to model different stages of abstraction >> which is not the matter of composition. Where/How do you think we should >> introduce composition? > > I'm talking about composition in the sense of the Gof "Strategy" pattern. > I think we can say that "James has different store strategies". > I also can be in line with you when you say the behaviour is not really > changed (we always talk about "storing" stuff). > However, fine-grained behaviour is quite different depending on the store > (jpa/jcr/file/nosql such as cassandra). > Another argument for composition in that underlying api can change without > disturbing the existing 3rd party store impl (think what will happen if you > rename a method in the abstract class). The enormeous advantage of > composition is that the interface of the composed class is the published > contract and should not change (between minor releases). So you can change > anything in the abstract without breaking 3rd party implementations. > > But I'd better take my IDE and to create a patch with a composed API.... > However, I've got a number of waiting JIRAs, and if you don't see anything > from me soon, let's not wait to implement this refactoring in the > inheritance way. > We can also come on this later on if appropriate. > >> Regards >> Tim >> >> >> Am Freitag, den 20.08.2010, 14:17 +0200 schrieb Eric Charles: >>> >>> Hi Norman, >>> >>> The following hierarchy is obvious and gives services such as create, >>> login, logout, search on *mailboxes*: >>> >>> MailboxManager (interface) >>> /|\ >>> | >>> DelegatingMailboxManagerimpl (abstract) >>> /|\ >>> | >>> StoreMailboxManager (abstract) >>> /|\ >>> | >>> JPA/JCR/Maildir/...MailboxManager (concrete) >>> >>> This one gives services to append, set flags', expunge on *messages*: >>> >>> Mailbox (interface) >>> /|\ >>> | >>> AbstractStoreMessageManager<Id> (abstract) >>> /|\ >>> | >>> StoreMessageManager<Id> (abstract) >>> /|\ >>> | >>> JPA/JCR/Maildir/...MessageManager (concrete) >>> >>> Wouldn't that Mailbox interface finally be named MessageManager ? >>> But np for me to live with the existing names. >>> >>> Will StoreMessageManager remain abstract (it is in the patch) ? (if yes, >>> we will end up with a AbstractStoreMessageManager and a >>> StoreMessageManager, both abstract,...). >>> I suppose the criteria to implement in AbstractStoreMessageManager was >>> 'no need for messageMapper'. Is there a common characteristic to these >>> methods (so we can QualifyStoreMessageManager) ? >>> >>> If I understand well, store implementors could reuse or simply override >>> methods of *StoreMessageManager. >>> >>> I was also talking about composition, so a store implementator would >>> know directly which methods he should implement to take control on a >>> certain behaviour aspect. >>> With the inheritance mecanism, the implementator has to go "deep" in the >>> class hierarchy to know it. >>> >>> Would you rename Document to Message in IMAP-202 ? >>> >>> (btw, in my first reply, I confused somewhere ...Mapper with ...Manager, >>> sorry) >>> >>> Tks, >>> >>> Eric >>> >>> >>> On 20/08/2010 12:07, Norman Maurer wrote: >>>> >>>> Comments inline.. >>>> >>>> >>>> 2010/8/20 Eric Charles<e...@apache.org>: >>>>> >>>>> OK >>>>> So AbstractStoreMessageManager doesn't implement MessageMapper (and it >>>>> was >>>>> in your javadoc :) >>>>> ... and implements Mailbox (the org.apache.james.imap.mailbox.Mailbox, >>>>> not >>>>> the org.apache.james.imap.store.mail.model.Mailbox...) >>>>> >>>> yep .. >>>> >>>>> The org.apache.james.imap.mailbox.Mailbox has much too do with >>>>> MaiboxSession >>>>> and not with domain classes, so it respects your goal. >>>>> Maybe the domain independence could be also documented in the javadoc. >>>>> >>>> Fair enough.. >>>> >>>>> Talking about domain model, I'm sometimes a bit confused. We have a >>>>> nice >>>>> package org.apache.james.imap.store.mail.model that contains the model, >>>>> but >>>>> my 2-cents: >>>>> - There are a few utils classes (PropertyBuilder,...) that could be >>>>> moved >>>>> somewhere else. >>>> >>>> I see no value here.. >>>> >>>>> - Document could be renamed to Message (I think we talked about it some >>>>> time >>>>> ago) >>>> >>>> +1 >>>> >>>>> - When you read code, Mailbox,... is sometimes confusing because it >>>>> exists >>>>> somewhere else, for example in org.apache.james.imap.mailbox.Mailbox. >>>>> This >>>>> last one has more to do with "service" than with "domain". I don't have >>>>> a >>>>> good name right now, but Maiboxable seems the closer to what I'm >>>>> looking >>>>> for. >>>>> >>>> Thats why its still called Mailbox ;) >>>> >>>>> Hope I'm not too niggling, >>>>> >>>>> Tks, >>>>> >>>>> Eric >>>>> >>>>> >>>>> On 20/08/2010 07:36, Norman Maurer wrote: >>>>>> >>>>>> Let me try to explain it.... >>>>>> >>>>>> The idea is to let the MessageMapper untouched, because its API is >>>>>> really easy to understand and use. Its not the most Performant todo >>>>>> but thats the price to pay. >>>>>> >>>>>> So I introduced a new abstract class which can be used if you want to >>>>>> write a custom store which is not forced to use a MessageMapper and so >>>>>> is a good fit for other implementations which not works so well with >>>>>> the Domain model and lazy loading (nosql for example). >>>>>> >>>>>> Hope this explains it... >>>>>> >>>>>> Thx >>>>>> Norman >>>>>> >>>>>> 2010/8/20, Eric Charles<e...@apache.org>: >>>>>>> >>>>>>> Hi Norman, >>>>>>> >>>>>>> I applied the last uploaded file (proposal_v3.diff): >>>>>>> >>>>>>> - patch works, which is good :) >>>>>>> >>>>>>> - I don't see any change on MessageMapper. Probably the modif to >>>>>>> return >>>>>>> only uid was already committed (IMAP-203,...) ? >>>>>>> >>>>>>> - You migrated methods from StoreMessageManager to >>>>>>> AbstractMessageMapper. So now, I'm a bit confused about the class >>>>>>> "responsiblity". Could you summary us the "roles/responsibilities" of >>>>>>> these 2 classes. That will help to define which methods goes where... >>>>>>> >>>>>>> - Abstract/extend is widely used in james. I sometimes force myself >>>>>>> to >>>>>>> thin to "favor composition over inheritance". Did you consider >>>>>>> composition instead of AbstractMessageMapper ? (maybe stupid question >>>>>>> and I should have tried to implement before asking... :) Of course, >>>>>>> you >>>>>>> can find plenty of discussions and explanations >>>>>>> (http://www.google.com/search?q=favor+composition+over+inheritance) >>>>>>> on >>>>>>> the net >>>>>>> >>>>>>> - I like the new UpdatedFlag class. When you read it, you understand >>>>>>> what it is aimed for. Separating "data domain" from "service domain" >>>>>>> is >>>>>>> always good. >>>>>>> >>>>>>> Tks, >>>>>>> >>>>>>> Eric >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 08/19/2010 12:20 PM, Norman Maurer wrote: >>>>>>>> >>>>>>>> Hi there, >>>>>>>> >>>>>>>> I' currently looking for ways to make it easier to create high >>>>>>>> performant Mailbox implementations while reusing the store api. I >>>>>>>> think there are some things we should change to allow this. >>>>>>>> >>>>>>>> * We should return only the uid of the message if nothing more is >>>>>>>> needed >>>>>>>> * We should move some stuff to an abstract MessageMapper >>>>>>>> implementation to make it easier to override and handle it different >>>>>>>> and still be able to use the store api. >>>>>>>> >>>>>>>> For this I opened a jira and attached a patch whic hshows the idea.. >>>>>>>> I'm still not 100% sure if its really a good idea, so feedback is >>>>>>>> welcome :) >>>>>>>> >>>>>>>> https://issues.apache.org/jira/browse/IMAP-202 >>>>>>>> >>>>>>>> Bye, >>>>>>>> Norman >>>>>>>> >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> 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 >>>>>>> >>>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> 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 >>>>> >>>>> >>>> Bye, >>>> Norman >>>> >>>> --------------------------------------------------------------------- >>>> 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 >>> >> >> --------------------------------------------------------------------- >> 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
