chemistry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Florian Müller <f...@apache.org>
Subject Re: Chunked transfer encoding
Date Sun, 12 May 2013 13:15:42 GMT
Hi Carlo,

I have refactored the server code.
Please have a look.

- Florian


> Hi Florian,
>
> the core of my change isn't in the Dispatcher itself. Discarding final
> classes, private constructors and static methods is the main objective
> of that change, as it basically enable class inheritance. Pluggability
> of different behavior can indeed be achieved by exposing the Dispatcher,
> but it would still be using the reflection over static methods combo as
> before, which alone breaks four out of five features of OO
> <http://en.wikipedia.org/wiki/Object-oriented_programming>[1].
>
> On the runtime efficiency, I'd be happy to store some 100 more class
> definition if that will spare some CPU cycles due to reflection. We're
> talking of one-off tiny memory allocation VS tiny performance
> degradation per every single call here.
>
> c.
>
> [1] please don't take me for an OO fanatic, I'm much more of a FP guy
> these days. I just think that OO is better than procedural, especially
> on an OO platforms such as Java
>
>
> 2013/5/10 Florian Müller <fmui@apache.org <mailto:fmui@apache.org>>
>
>     Got it. I thought you were heading in a different direction.
>
>     How is that different from making the current Dispatch object
>     protected or adding a protected method that wraps the
>     Dispatch.addResource() method?
>
>     Extensibility-wise they would be equal. Your version only adds a bit
>     more syntactic sugar. It would also need a bit more memory because
>     we would have to add about 100 additional classes (AtomPub +
>     Browser) and runtime objects.
>
>     - Florian
>
>
>         Is it next week already? :-)
>
>         Jokes apart, I had one hour to spare for this topic now that
>         it's still
>         hot, and produced the patch you can see attached to
>         CMIS-656<https://issues.__apache.org/jira/browse/CMIS-__656
>         <https://issues.apache.org/jira/browse/CMIS-656>>.
>
>         That's a translation of the current Chemistry dispatch logic to
>         abandon
>         reflection, static methods, private constructors, and final classes.
>
>         The flow is quite the same as before, with the servlet
>         initialization logic
>         registering the default bindings by adding concrete instances of a
>         ServiceCall interface into a local registry. At request serving
>         time the
>         registered ServiceCall is retrieved and its service() method
>         invoked. The
>         invocation logic is now a mere INVOKEINTERFACE JVM instruction
>         instead of
>         the previous reflection trick (tiny performance improvement here).
>
>         Please note that the ServiceCall interface was already a hidden
>         contract
>         that all service calls had to implement to be addressable by the
>         Method
>         created by the Dispatcher.
>
>         As a last note, CmisAtomPubServlet also exposes a protected
>         registerDispatch(resource, method, ServiceCall) to allow
>         implementors to
>         override the default logic.
>
>         That's it for now. Please note that as the Dispatcher is shared
>         across the
>         AtomPub and Browser bindings, and as only the AtomPub was properly
>         refactored, that patch will break your build.
>
>         Looking forward to hear your comments,
>         c.
>
>
>         2013/5/10 Carlo Sciolla<carlo.sciolla@gmail.__com
>         <mailto:carlo.sciolla@gmail.com>>
>
>             Hi Florian,
>
>             will do, I'll hopefully have a patch for you to review next
>             week.
>
>             c.
>
>
>             2013/5/10 Florian Müller<fmui@apache.org
>             <mailto:fmui@apache.org>>
>
>                 Hi Carlo,
>
>                 Please go ahead and make a proposal. Submit a patch that
>                 sketches your
>                 ideas. It doesn't have to be complete and running.
>                 It's easier to talk about something concrete rather than
>                 discussing about
>                 generic topics.
>
>                 - Florian
>
>
>
>                    Hi Florian,
>
>                     it's not my intent to start a flame war, and we can
>                     continue offline if
>                     you
>                     want. To the interest of the list, I'll just write
>                     here some of the
>                     reasons
>                     behind my previous comment, and wait until I have
>                     some patch ready to
>                     prove
>                     my points before bothering you guys again with my
>                     ramblings.
>
>                     I understand your remark on the spec compliance, and
>                     that interop is at
>                     stake when you allow the protocol details to be
>                     modified. BUT...
>
>                     - There's much more to HTTP than what CMIS
>                     specifies, and Chemistry is
>                     currently in charge of handling the transport
>                     protocol on top of which
>                     CMIS
>                     expresses itself.
>
>                     - No extensibility means that backporting of fixes
>                     might be a no-go. For
>                     example, if I now upgrade Chemistry to the latest
>                     trunk my code doesn't
>                     compile anymore. Subclassing would be the best way
>                     to integrate your
>                     changes in the version I use, but that's not
>                     available to me.
>
>                     - The CMIS spec is (hopefully!) not fixed and will
>                     evolve with time. OO
>                     could help greatly e.g. to support different
>                     versions of the protocol
>                     with
>                     a single serve instance, which is painful to
>                     implement cleanly following
>                     the current code approach.
>
>                     - Spec extensions are not evil per se. While they
>                     might indeed break
>                     interoperability, they're also a way to open the
>                     protocol itself to
>                     experiments and eventually drive a community based
>                     evolution of it.
>
>                     - Interop is not always the top priority. CMIS and
>                     Chemistry provide a
>                     lot
>                     of value OOTB, and I don't see why proprietary
>                     extensions should be
>                     denied
>                     by principle. Why shouldn't I have the possibility
>                     to write a custom
>                     extension to improve e.g. content delivery capabilities
>                     (getContentStreamByPath, anyone?), while still
>                     retaining full protocol
>                     compliance for B2B data exchange?
>
>                     If you made it to here, a big thank you for your
>                     patience.
>
>                     Hope this helps,
>                     c.
>
>
>
>
>                     2013/5/8 Florian Müller<fmui@apache.org
>                     <mailto:fmui@apache.org>>
>
>                        Hi Carlo,
>
>                         This code is not meant to be extensible. The
>                         CMIS standard is fixed.
>                         There
>                         will be no additional services or operations.
>                         Also, the semantics of the
>                         parameters will not change or have to be
>                         changed. Any extensions would
>                         bypass the specification, which would be a pain
>                         for clients and doesn't
>                         help interoperability.
>                         All methods that handle requests are stateless,
>                         isolated pieces of code.
>                         OO doesn't help here. And the alternative to
>                         reflections would be a 120
>                         lines if-statement. I can't see the advantage of
>                         that.
>
>                         Also, this part is considered internal code and
>                         may change at any time.
>                         OpenCMIS provides a lot of extensibility on the
>                         client side and keeping
>                         the
>                         interfaces compatible does hurt sometimes. I
>                         don't see a real use case
>                         to
>                         have that pain also on the server side. If there
>                         is something missing or
>                         wrong, we usually fix it pretty quick - as you
>                         have seen.
>
>                         Having said that, if you have a great idea how
>                         to refactor that code,
>                         feel
>                         free to provide a patch.
>
>
>                         - Florian
>
>
>                         Am Mittwoch, den 08.05.2013, 16:05 +0200 schrieb
>                         Carlo Sciolla<
>                         carlo.sciolla@gmail.com
>                         <mailto:carlo.sciolla@gmail.com>>:
>
>                            Hi Florian,
>
>                             thanks for your quick commit, I will
>                             experiment a bit with it and let
>                             you
>                             know what comes out of it. I do already have
>                             some initial comments
>                             anyway:
>
>                             - I see you only addressed the browser
>                             bindings implementation. While I
>                             can
>                             see the reason behind it, I think it won't
>                             hurt to also apply a similar
>                             logic to the AtomPub binding
>
>                             - as I'm stuck with v0.6.0, I'm looking into
>                             ways to backport or
>                             integrate
>                             your code in my app. The current logic for
>                             method dispatch in AtomPub
>                             goes
>                             against extensibility (and some object
>                             oriented design principles, IMO)
>                             and
>                             while for the time being I can work around
>                             it, would you guys consider
>                             refactoring the dispatch logic to make use
>                             of non-final classes / no
>                             reflection / public constructors?
>
>                             Thanks,
>                             c.
>
>
>                             2013/5/8 Florian Müller<fmui@apache.org
>                             <mailto:fmui@apache.org>>
>
>                                Hi Carlo,
>
>                                 I've added some new code. There are now
>                                 three interfaces that let you
>                                 control the server headers.
>                                 The ContentStream object that is
>                                 returned by getContentStream() must
>                                 implement the interface(s) to trigger
>                                 the behavior:
>
>                                 ContentLengthContentStream - Sets the
>                                 Content-Length header and turns
>                                 chunking off.
>
>                                 LastModifiedContentStream - Sets the
>                                 Last-Modified header and respects
>                                 the
>                                 If-Modified-Since header.
>
>                                 CacheHeaderContentStream - Sets the
>                                 Cache-Control header, the Expires
>                                 header, and the ETag header and respects
>                                 the If-None-Match header.
>
>
>                                 Please let me know if that works for you.
>
>
>                                 - Florian
>
>
>                                    Hi there, sorry for the late reply.
>
>
>                                     2013/5/7 Florian
>                                     Müller<fmui@apache.org
>                                     <mailto:fmui@apache.org>>
>
>                                        That is surprising. Chunked
>                                     encoding isn't really exotic.
>
>
>                                            Definitely, but browsers are
>                                         always there to remind us that world
>                                         class
>
>                                            standards are nothing
>                                         different from regional social
>                                         conventions,
>
>                                     are
>                                     they?
>
>
>                                        Could you please open an
>                                     Improvement issue and add a few details.
>                                     I'll
>
>                                        look into it.
>
>                                            Thanks, here it
>                                         is<https://issues.apache.org/*__*****
>                                         <https://issues.apache.org/******><https://issues.apache.__org/****
>                                         <https://issues.apache.org/****>>
>
>                                         jira/browse/CMIS-655<https://*__*issues.apache.org/**jira/**
>                                         <http://issues.apache.org/**jira/**>
>                                         browse/CMIS-655<https://__issues.apache.org/**jira/__browse/CMIS-655
>                                         <https://issues.apache.org/**jira/browse/CMIS-655>>>
>
>                                         <https://**issues.apache.org/*__*jira/browse/**CMIS-655
>                                         <http://issues.apache.org/**jira/browse/**CMIS-655><http:/__/issues.apache.org/jira/__browse/**CMIS-655
>                                         <http://issues.apache.org/jira/browse/**CMIS-655>>
>                                         <https:/**/issues.apache.org/__jira/**browse/CMIS-655
>                                         <http://issues.apache.org/jira/**browse/CMIS-655><https:/__/issues.apache.org/jira/__browse/CMIS-655
>                                         <https://issues.apache.org/jira/browse/CMIS-655>>
>
>
>                                          >.
>
>
>
>
>
>             --
>             Carlo Sciolla
>
>             --==(A)==--
>             Linux User #372086
>             My personal blog: http://www.skuro.tk
>             Follow me on twitter: http://twitter.com/skuro
>             <http://twitter.com/skuro>Fork me on Github:
>             http://github.com/skuro
>             <http://github.com/skuro>My LinkedIn profile:
>             http://nl.linkedin.com/in/__carlosciolla
>             <http://nl.linkedin.com/in/carlosciolla>
>             --==(A)==--
>
>             Product Lead at Backbase - Next Generation Portal Software
>             for Financials
>             &  Large Enterprises (http://www.backbase.com)
>
>
>
>
>
>
>
>
> --
> Carlo Sciolla
>
> --==(A)==--
> Linux User #372086
> My personal blog: http://www.skuro.tk
> Follow me on twitter: http://twitter.com/skuro
> <http://twitter.com/skuro>Fork me on Github: http://github.com/skuro
> <http://github.com/skuro>My LinkedIn profile:
> http://nl.linkedin.com/in/carlosciolla
> --==(A)==--
>
> Product Lead at Backbase - Next Generation Portal Software for
> Financials & Large Enterprises (http://www.backbase.com)


Mime
View raw message