On Fri, 20 Jan 2012, Gregory Farnum wrote:
> On Fri, Jan 20, 2012 at 10:58 AM, Sage Weil <sage@xxxxxxxxxxxx> wrote:
> > The current pattern is
> >
> > void Foo::encode(bufferlist& bl, unsigned features) const
> > {
> > if (features & NEW_THING) {
> > // encode normally
> > } else {
> > // encode old way (wahtever was in this func)
> > }
> > }
> >
> > I'm not sure what to add here to make this less painful or error-prone.
> > ceph-dencoder does have options to print the feature set for the version
> > it was compiled for, and to set the bits passed to encoder functions, so
> > that we can test these paths.
>
> Well, our features tend to all move forward, so perhaps we should have
> a way of specifying different encoders based on the feature bits (or
> struct version) we're targeting, instead of if-elsing through the code
> (nesting with something like this tends to sprawl ever-deeper). I
> think that would make it easier to test inter-version compatibility
> and avoid accidentally breaking interoperation.
>
> Not having spent a bunch of time thinking about how to do this I don't
> have obviously better ideas off-hand, but I'd really like it if
> somebody could either present a good option or else say why we won't
> get anything much better, instead of just pressing ahead because
> nobody has any better ideas. :( We're going to be stuck with this for
> a while.
Hmm. I don't like obscuring control flow with macros, and features needs
to be an argument. I don't have any real ideas here.
> >> > This requires and initial incompat change to add the length and incompat
> >> > fields, but then we can generally add things without breakage.
> >> Hmm...if we come up with something that talks feature bits we could do
> >> this nicely with a compat bit. :)
> >
> > Hmm, yeah. If we redo a bunch of encoders at once we can use a
> > single feature bit for all of them. That's a good idea!
>
> I'm not sure my thought came across clearly ? we should be able to use
> a compat rather than an incompat flag if we have a good way to target
> multiple versions in our encoder.
You'll need to explain what you mean on Monday.. I'm missing it!
> > I cleaned up the tool usage a bit and it currently looks like
> >
> > $ ./ceph-dencoder -h
> > usage: ceph-dencoder [commands ...]
> >
> > version print version string (to stdout)
> >
> > import <encfile> read encoded data from encfile
> > export <outfile> write encoded data to outfile
> >
> > features <num> set feature bits used for encoding
> > get_features print feature bits (int) to stdout
>
> We will probably be happier in the long run if we can use symbolic
> names for the features. Not sure how difficult that would be, though.
Just a bunch of string parsing and conversion functions. Meh...
> > list_types list supported types
> > type <classname> select type
> > decode decode into in-core object
> > encode encode in-core object
> > dump_json dump in-core object as json (to stdout)
> >
> > count print number of generated test objects (to stdout)
> > select <n> select generated test object as in-core object
>
> I'm not sure what "in-core" means here, nor what it means to "select type".
I'll it to 'in-memory'. :)
