[PLIP-Advisories] Re: [Plone] #7822: Make standard file content types use ZODB BLOB support
plip-advisories at lists.plone.org
plip-advisories at lists.plone.org
Wed Sep 30 14:39:56 UTC 2009
#7822: Make standard file content types use ZODB BLOB support
Reporter: limi | Owner: witsch
Type: PLIP | Status: assigned
Priority: major | Milestone: 4.0
Component: Infrastructure | Resolution:
Keywords: focusarea |
Replying to [comment:42 alecm]:
> (In ) Add reviw for PLIP #7822 (refs #7822)
quoting parts of [browser:buildouts/plone-
coredev/branches/4.0/plips/plip7822-review-alecm.txt at 29529 alec's review]
to be able to provide comments inline:
> Users of the product seem to speak favorably of it.
very nicely put! :)
> 1) The implementer has failed to include a document describing the
> PLIP along with the buildout.
my bad. i caught up just now, see r30062.
> 2) plone.app.blob tests do not pass under Plone 4.0 + Zope 2.12,
> though they appear to be well designed and offer reasonable
they pass now, albeit i had to downgrade to ZODB 3.8.3 for the time being
(please see r29981).
> 3) The Plone default profile has not been updated to install the BLOB
> content types by default.
fixed in r30057.
> 4) The BLOB Image type install profile does not appear in the Quick
quick-installer only supports exposes one profile per package. as image
support is not quite ready yet for general consumption, and since i'm
trying to provide a stable add-in for Plone 3.x (i.e. without branching
things for Plone 4) the "image replacement" profile isn't applied by
default yet. once image support is ready, the "default" profile will
start depending on it.
for now you'll either have to apply the profile manually via
`portal_setup` for Plone 3.x — for testing/reviewing in Plone 4.0 it
should get applied by default (again see r30057)
> 5) Two BLOB File replacement install profiles appear in portal_setup
one of those is actually registered for `ITestCasePloneSiteRoot`, but
still exposed by quick-installer. i'm not sure if this is intended
behaviour, but the distinction should be clearer as of r29885.
> 6) The tests (which again don't pass) depend on
> collective.monkeypatcher, but the buildout does not.
fixed in r29685, thanks martin!
> 7) The product relies on monkeypatching (ZODB.Blob and
> ZPublisher.HTTPRequest). These patches should be integrated into
> the Zope core if we are going to include this product.
the patch for `setstate_wrapper` can go (as the comments state). i've
already made the necessary fix in ZODB, which was subsequently released in
the patches for `HTTPRequest` actually originated in Zope 2.11. however,
the original fix doesn't work on windows, and the working version from
`plone.app.blob` hasn't been pushed upstream yet. as this is a bug fix,
though, i don't see what would prevent it from being accepted for 2.12.1
(also see r19124 for more info).
> 8) The product depends on plone.app.imaging which monkeypatches
> Archetypes.Field.ImageField, this patch should be integrated if
yes, it should. i've added it to the 2dos.
> Issues and Concerns [[br]]
> 1) Performance: When serving a small image (600 B) I can get ~108
> req/sec with ATFile, but only ~80 req/sec with a BLOB File.
this was mostly due to a subtle performance issue in
`archetypes.schemaextender`, which has been fixed in r11344 (in the
archetypes repo). with that fix such small files are served at about 100
req/sec, leaving a performance penalty of less than 10%.
imho, it's rather unlikely that many small files (i.e. <1k) will be stored
in actual "File" or "Image" content, but some generated image scales might
be this small, of course. possible solutions to this are to either store
image scales as attributes like before, which can be achieved by simply
disabling the extra adapter provided by `plone.app.blob`, turning the same
adapter into a local adapter which can then be enabled separately, or else
by only storing the payload data on blobs from a certain size, e.g. 16k.
the last option would probably be best in all cases, but i'm not sure the
extra effort (and added complexity) is really worth it. therefore i will
conduct some more benchmarks and provide the results for the FWT to
> 2) If versioning is enabled for Files, reverting to another version
> removes the file content.
the versioning issues are indeed the ones that worry me most — i've looked
into resolving them before, but gave up at some point. i hope that there
will be a chance to look into this with alec and/or other people in
budapest. until then versioning will have to remain disabled for blob-
based content, which luckily also is the default in Plone.
> 3) The package depends on archetypes.schemaextender, which is a
> worthwhile add-on package in wide use. However, the package
> introduces a significant potential decrease in performance (see
> PLIP #9376 and reviews).
please see above. the fix in r11344 (in the archetypes repo) resolves the
performance issues in `archetypes.schemaextender` almost completely. with
the fix only the actually added fields will have a performance impact,
which is — according to my profiling and benchmarks — mostly neglectable,
though. please note that this should also render PLIP #9376 mostly
> 4) Image scales appear to be generated on the fly and not stored
this was fixed in r29953. i will provide updated benchmark results.
> With some work to properly integrate this package into Plone 4.0 and
> some further discussion of the potential problems listed above, I
> would strongly consider recommending this PLIP for inclusion in Plone.
all remaining issues i consider to be critical before releasing 1.0 (to be
included into Plone 4.0) are now listed in the [browser:buildouts/plone-
coredev/branches/4.0/plips/plip7822-blob-support.txt review notes]. there
are still a number of bug fixes required, especially the one regarding
versioning, but most of the current test failures in `Plone` and
`ATContentTypes` are due to the new approach used for image scales (i.e.
the traversal adapter vs attributes).
Ticket URL: <http://dev.plone.org/plone/ticket/7822#comment:61>
Plone Content Management System
More information about the PLIP-Advisories