Re: Convention around encode/decode versions

[Yixin Jin <yjin77@xxxxxxxx>]

Thanks, Eric, for the confirmation and clarification. That was great! :)

Yixin

On Monday, July 24, 2023 at 10:31:34 a.m. EDT, J. Eric Ivancich <ivancich@xxxxxxxxxx> wrote:

I think you’re right that currently it’s dead code. However I think there’s a strategy to make the minimal changes necessary in encode/decode when adding a new version to the datatype. That serves two purposes. It makes mistakes that would be hard to recover from much less likely. And it helps document the history of the datatype.

So in the example you provide it appears that prior to version 6 the datatype only allowed a single access and secret key, but starting with version 6 multiple keys were enabled.

Eric
(he/him)

> On Jul 20, 2023, at 2:35 PM, Yixin Jin <yjin77@xxxxxxxx> wrote:
>
> Hi folks,
>
> I noticed that the encode/decode functions enforce versions in order to achieve backward compatibility and provide a upgrade path forward. However, I'd like to confirm the standard of practice around the use of versions in this case. If decode() function states the compatv is 9, there should be no code inside that handles the case of struct_v < 9, since this condition should never be satisfied. Is this the right understanding? I saw this block of code in RGWUserInfo::decode():
>
>  void decode(bufferlist::const_iterator& bl) {
>        DECODE_START_LEGACY_COMPAT_LEN_32(22, 9, 9, bl);                                                                               
>        if (struct_v >= 2) {                                                                                                           
>          uint64_t old_auid;
>          decode(old_auid, bl);                                                                                                       
>        }
>        std::string access_key;                                                                                                       
>        std::string secret_key;                                                                                                       
>      decode(access_key, bl);                                                                                                         
>      decode(secret_key, bl);                                                                                                         
>      if (struct_v < 6) {                                                                                                             
>        RGWAccessKey k;
>        k.id = access_key;
>        k.key = secret_key;
>        access_keys[access_key] = k;                                                                                                 
>      }
>
> I don't see why we need to handle the case of struct_v < 6 when compatv is 9. Is it safe to assume that this if statement is a dead code? If so, could we also assume that the following if block in its encode() function should be removed, too?
>
>  void encode(bufferlist& bl) const {
>        ENCODE_START(22, 9, bl);
>        encode((uint64_t)0, bl); // old auid
>        std::string access_key;
>        std::string secret_key;
>        if (!access_keys.empty()) {
>          std::map<std::string, RGWAccessKey>::const_iterator iter = access_keys.begin();
>          const RGWAccessKey& k = iter->second;
>          access_key = k.id;
>          secret_key = k.key;
>        }
>
>
>
> Thanks,
> Yixin

> _______________________________________________
> Dev mailing list -- dev@xxxxxxx
> To unsubscribe send an email to dev-leave@xxxxxxx

_______________________________________________
Dev mailing list -- dev@xxxxxxx
To unsubscribe send an email to dev-leave@xxxxxxx