objectstore: don't build unless asked for

ellie timoney ellie at fastmail.com
Fri Nov 6 02:44:52 EST 2015


At present, our master branch unconditionally builds in the object
storage functionality (using the objectstore_dummy implementation). 
There's a few ways this isn't great:

* it has an undocumented dependency on lib/sqldb, but doesn't try to
link it in, so the build fails unless you --enable-httpd, which is
currently the only thing that tries to link in lib/sqldb.  (This is
what's causing the HarborMaster build failures, fwiw.)
* it brings in a bunch of experimental code with no way to compile that
out
* there's some configure settings for dealing with it, but they don't
interact well together

I had a chat with Ken and Nicola today, and have put together some
changes, with the following goals:

* as light a touch as reasonable
* add an --enable-objectstore configure option (default: no) to control
whether objectstore code is built at all
* keep the existing --with-openio[...] and --with-caringo options for
selecting the backend
* ditch the --enable-dummy_objstore option altogether
* use the dummy implementation only if objectstore is enabled but
neither backend has been selected

The changes are on the "v30/optional-objectstore" branch on my github,
for now:
https://github.com/elliefm/cyrus-imapd/tree/v30/optional-objectstore

This isn't on master yet cause there's some caveats:

The implementation necessitates a bunch of "#ifdef ENABLE_OBJECTSTORE"
around the code paths that use it (imap/append.c and imap/mailbox.c).  I
don't much care for this -- it makes maintenance and testing down the
track fiddly.  But the alternatives seem to be the current state, which
we already know is broken, or adding a no-op objectstore implementation.
 The latter seems pretty nasty from the way we use this: there's lots of
code like "if the option is set in imapd.conf, do it the objectstore
way, otherwise, do it the normal way".  If we were to just drop in a
no-op objectstore implementation then cyrus admins would be only a
config setting away from data loss, and that feels all kinds of wrong. 
So, #ifdefs.

The dummy implementation is not a no-op implementation, it's a flat
file-based emulation.  "Dummy" is possibly a misleading name here, by
which I mean, it mislead me until I read the code.  Maybe we should
rename it, but for now: light touch.

The caringo/openio detection code is pretty frail and needs attention. 
For example, if you --with-caringo=yes without having the dependencies
installed, it just happily adds the dependencies straight into
LDFLAGS(!)... which makes subsequent unrelated checks that try to
compile a code fragment fail unexpectedly.  The openio path has the good
grace to fail clumsily on its own when the dependencies are missing,
though it is just as guilty of blatting into stuff it shouldn't touch. 

The --enable-objectstore option still has the undocumented dependency on
--enable-http (in order to bring in lib/sqldb).  Light touch.  I'll end
up fixing this at some point anyway if no-one else does first, because
the backup code I'm working on has a similar dependency.

I'm not sure about the terminology here -- "object store" / "object
storage" / various capitalisations / without spaces.  I've just made
some wild guesses for now.  If you have an opinion, tell me about it.

The imapd.conf setting for enabling object storage is always present,
whether the feature is compiled in or not.  There's a fair bit of work
in making this not the case (changing the scripts that generate
lib/imapopts.[ch] and man/imapd.conf.5 from lib/imapoptions).  And we
already have other settings that are nonsensical unless optional
features are compiled in, so there's precedent in just leaving it as is
-- at least for now. Light touch.

And for all that, I don't have the dependencies for either Caringo or
OpenIO handy, so I haven't tested those code paths beyond verifying that
it fails as expected when I try to enable them.  I've tried not to alter
what was already there, but I can't verify that I haven't accidentally
broken something somehow.  If you've been able to get either of these
actually working, I'd appreciate if you could try again with my branch
and let me know if it still works.  It does pass our tests without
objectstore, and with the dummy objectstore implementation.

So, those sure are some caveats hey.

I think the main questions at this point are:

* are we happy with the combination of --enable-objectstore to turn the
feature on, plus --with-[backend] to select a backend, and default to
dummy if no other backend was selected?
* are we happy/comfortable/resigned to the #ifdef'd approach to
compiling it out?
* did I manage to avoid breaking anything that used to work?

If those are yes, then I'll merge it onto master, and the rest can be
figured out as we go.

Cheers,

ellie


More information about the Cyrus-devel mailing list