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.
>> > 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.
> 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.
> 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".
The rest looks good to me, though. :)
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
