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 <[email protected] <mailto:[email protected]>>
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:[email protected]>>
Hi Florian,
will do, I'll hopefully have a patch for you to review next
week.
c.
2013/5/10 Florian Müller<[email protected]
<mailto:[email protected]>>
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<[email protected]
<mailto:[email protected]>>
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<
[email protected]
<mailto:[email protected]>>:
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<[email protected]
<mailto:[email protected]>>
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<[email protected]
<mailto:[email protected]>>
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)
