[wip-addr-features] make sure I am doing the right thing

[wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

Hi Sage, Haomai,

I am working on the require-features-patch, I use 'make' to see the conflicts,
and change the code to fit the required-feature-patch, [1] is a huge patch
that I am still working on, it has not been finished. I really need to check
with you whether I am doing it right, since it seems the errors are endless.

There are some comments where I am not sure in the patch, like 'not sure'.

I didn't split this huge patch into small ones, I am not sure is that a must, if
yes, I will split it into small ones.

Any comments and critisize are welcomed.

[1]https://github.com/zhjwpku/ceph/commit/706ea2eca3bf8849bd4c2ff0cd9757318c6bbb14

Thanks!
Zhao

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

Hi Junwang,

On Mon, 16 May 2016, Junwang Zhao wrote:
> Hi Sage, Haomai,
> 
> I am working on the require-features-patch, I use 'make' to see the conflicts,
> and change the code to fit the required-feature-patch, [1] is a huge patch
> that I am still working on, it has not been finished. I really need to check
> with you whether I am doing it right, since it seems the errors are endless.
> 
> There are some comments where I am not sure in the patch, like 'not sure'.

I skimmed through this and it looks mostly right, but I see a few cases 
where features aren't needed, e.g. cls_refcount_get in 
cls_refcount_client.cc (there's no addr being encoded in 
cls_refcount_get_op, so no need to make the encoding featureful).
 
> I didn't split this huge patch into small ones, I am not sure is that a 
> must, if yes, I will split it into small ones.

The end result needs to be a series of small patches, but that doesn't 
have to happen right away.  I think it might be useful to do a few small 
sets of changes first just to show what the goal is, though.

I will take the patch below and pull a few sample changes out so you can 
see.  Traveling at the moment, but I'll have something pushed today (that 
is also rebased on top of the latest wip-addr-cleanup branch).

Are you on IRC in #ceph-devel?  What is your nick?

Thanks!
sage

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

On Mon, 16 May 2016, Sage Weil wrote:
> Hi Junwang,
> 
> On Mon, 16 May 2016, Junwang Zhao wrote:
> > Hi Sage, Haomai,
> > 
> > I am working on the require-features-patch, I use 'make' to see the conflicts,
> > and change the code to fit the required-feature-patch, [1] is a huge patch
> > that I am still working on, it has not been finished. I really need to check
> > with you whether I am doing it right, since it seems the errors are endless.
> > 
> > There are some comments where I am not sure in the patch, like 'not sure'.
> 
> I skimmed through this and it looks mostly right, but I see a few cases 
> where features aren't needed, e.g. cls_refcount_get in 
> cls_refcount_client.cc (there's no addr being encoded in 
> cls_refcount_get_op, so no need to make the encoding featureful).
>  
> > I didn't split this huge patch into small ones, I am not sure is that a 
> > must, if yes, I will split it into small ones.
> 
> The end result needs to be a series of small patches, but that doesn't 
> have to happen right away.  I think it might be useful to do a few small 
> sets of changes first just to show what the goal is, though.
> 
> I will take the patch below and pull a few sample changes out so you can 
> see.  Traveling at the moment, but I'll have something pushed today (that 
> is also rebased on top of the latest wip-addr-cleanup branch).

I went on a bit of a binge and have a wip-addr-work branch that is mostly 
there.  The last thing (I think) is updating msg/simple and msg/async.  
These ones should explicitly encode with features 0 since the protocol is 
defined in terms of the legacy encoding.

sage

Re: [wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

Hi Sage,

Check this[1], we should I do next, entity_addrvec_t ?

[1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
> On Mon, 16 May 2016, Sage Weil wrote:
>> Hi Junwang,
>>
>> On Mon, 16 May 2016, Junwang Zhao wrote:
>> > Hi Sage, Haomai,
>> >
>> > I am working on the require-features-patch, I use 'make' to see the conflicts,
>> > and change the code to fit the required-feature-patch, [1] is a huge patch
>> > that I am still working on, it has not been finished. I really need to check
>> > with you whether I am doing it right, since it seems the errors are endless.
>> >
>> > There are some comments where I am not sure in the patch, like 'not sure'.
>>
>> I skimmed through this and it looks mostly right, but I see a few cases
>> where features aren't needed, e.g. cls_refcount_get in
>> cls_refcount_client.cc (there's no addr being encoded in
>> cls_refcount_get_op, so no need to make the encoding featureful).
>>
>> > I didn't split this huge patch into small ones, I am not sure is that a
>> > must, if yes, I will split it into small ones.
>>
>> The end result needs to be a series of small patches, but that doesn't
>> have to happen right away.  I think it might be useful to do a few small
>> sets of changes first just to show what the goal is, though.
>>
>> I will take the patch below and pull a few sample changes out so you can
>> see.  Traveling at the moment, but I'll have something pushed today (that
>> is also rebased on top of the latest wip-addr-cleanup branch).
>
> I went on a bit of a binge and have a wip-addr-work branch that is mostly
> there.  The last thing (I think) is updating msg/simple and msg/async.
> These ones should explicitly encode with features 0 since the protocol is
> defined in terms of the legacy encoding.
>
> sage

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

On Tue, 17 May 2016, Junwang Zhao wrote:
> Hi Sage,
> 
> Check this[1], we should I do next, entity_addrvec_t ?
> 
> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

Looks good!  I opened the PR

	https://github.com/ceph/ceph/pull/9184

for review and test.

The next step is just to make a new entity_addr_t encoding that is more 
compact and uses an ADDR2 feature.  One commit that defines the new 
feature, then one commit that changes the entity_addr_t encode/decode, 
somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use 
the new get_sockaddr_len() method for elen, though.

Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the 
transport version (IP version), which is already covered by the sa_family 
field in the sockaddr.  I think that makes sense...

A third commit would then add the entity_addrvec_t type, similar to what 
is in that same commit.  It should also add the type to 
test/encoding/types.h.

Thanks!
sage



> 
> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
> > On Mon, 16 May 2016, Sage Weil wrote:
> >> Hi Junwang,
> >>
> >> On Mon, 16 May 2016, Junwang Zhao wrote:
> >> > Hi Sage, Haomai,
> >> >
> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
> >> > that I am still working on, it has not been finished. I really need to check
> >> > with you whether I am doing it right, since it seems the errors are endless.
> >> >
> >> > There are some comments where I am not sure in the patch, like 'not sure'.
> >>
> >> I skimmed through this and it looks mostly right, but I see a few cases
> >> where features aren't needed, e.g. cls_refcount_get in
> >> cls_refcount_client.cc (there's no addr being encoded in
> >> cls_refcount_get_op, so no need to make the encoding featureful).
> >>
> >> > I didn't split this huge patch into small ones, I am not sure is that a
> >> > must, if yes, I will split it into small ones.
> >>
> >> The end result needs to be a series of small patches, but that doesn't
> >> have to happen right away.  I think it might be useful to do a few small
> >> sets of changes first just to show what the goal is, though.
> >>
> >> I will take the patch below and pull a few sample changes out so you can
> >> see.  Traveling at the moment, but I'll have something pushed today (that
> >> is also rebased on top of the latest wip-addr-cleanup branch).
> >
> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
> > there.  The last thing (I think) is updating msg/simple and msg/async.
> > These ones should explicitly encode with features 0 since the protocol is
> > defined in terms of the legacy encoding.
> >
> > sage
> 
> 

Re: [wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

Hi Sage, Haomai,
Please check [1] and give some feedback, I am not sure about
52436ded943b8f567adb584c50b789431b788a85

[1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

Thanks!

On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@newdream.net> wrote:
> On Tue, 17 May 2016, Junwang Zhao wrote:
>> Hi Sage,
>>
>> Check this[1], we should I do next, entity_addrvec_t ?
>>
>> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>
> Looks good!  I opened the PR
>
>         https://github.com/ceph/ceph/pull/9184
>
> for review and test.
>
> The next step is just to make a new entity_addr_t encoding that is more
> compact and uses an ADDR2 feature.  One commit that defines the new
> feature, then one commit that changes the entity_addr_t encode/decode,
> somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use
> the new get_sockaddr_len() method for elen, though.
>
> Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the
> transport version (IP version), which is already covered by the sa_family
> field in the sockaddr.  I think that makes sense...
>
> A third commit would then add the entity_addrvec_t type, similar to what
> is in that same commit.  It should also add the type to
> test/encoding/types.h.
>
> Thanks!
> sage
>
>
>
>>
>> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
>> > On Mon, 16 May 2016, Sage Weil wrote:
>> >> Hi Junwang,
>> >>
>> >> On Mon, 16 May 2016, Junwang Zhao wrote:
>> >> > Hi Sage, Haomai,
>> >> >
>> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
>> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
>> >> > that I am still working on, it has not been finished. I really need to check
>> >> > with you whether I am doing it right, since it seems the errors are endless.
>> >> >
>> >> > There are some comments where I am not sure in the patch, like 'not sure'.
>> >>
>> >> I skimmed through this and it looks mostly right, but I see a few cases
>> >> where features aren't needed, e.g. cls_refcount_get in
>> >> cls_refcount_client.cc (there's no addr being encoded in
>> >> cls_refcount_get_op, so no need to make the encoding featureful).
>> >>
>> >> > I didn't split this huge patch into small ones, I am not sure is that a
>> >> > must, if yes, I will split it into small ones.
>> >>
>> >> The end result needs to be a series of small patches, but that doesn't
>> >> have to happen right away.  I think it might be useful to do a few small
>> >> sets of changes first just to show what the goal is, though.
>> >>
>> >> I will take the patch below and pull a few sample changes out so you can
>> >> see.  Traveling at the moment, but I'll have something pushed today (that
>> >> is also rebased on top of the latest wip-addr-cleanup branch).
>> >
>> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
>> > there.  The last thing (I think) is updating msg/simple and msg/async.
>> > These ones should explicitly encode with features 0 since the protocol is
>> > defined in terms of the legacy encoding.
>> >
>> > sage
>>
>>

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

On Thu, 19 May 2016, Junwang Zhao wrote:
> Hi Sage, Haomai,
> Please check [1] and give some feedback, I am not sure about
> 52436ded943b8f567adb584c50b789431b788a85
> 
> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

Great! I made several suggestions, and I have a question for everyone: 
what should we do if we have a "new" addr (say, for the v2 msgr protocol) 
and we need to encode it for a client that doesn't have the ADDR2 feature?
Right now we are still stuffing the sockaddr_storage in their with 
whatever sockaddr data we have, but it is indistinguishable from a legacy 
address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or 
blank, except with a magic nonce (0xffffffff maybe) so that you can tell 
that it is non-blank but not terribly useful/usable?

sage


 
> > Thanks!
> 
> On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@newdream.net> wrote:
> > On Tue, 17 May 2016, Junwang Zhao wrote:
> >> Hi Sage,
> >>
> >> Check this[1], we should I do next, entity_addrvec_t ?
> >>
> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
> >
> > Looks good!  I opened the PR
> >
> >         https://github.com/ceph/ceph/pull/9184
> >
> > for review and test.
> >
> > The next step is just to make a new entity_addr_t encoding that is more
> > compact and uses an ADDR2 feature.  One commit that defines the new
> > feature, then one commit that changes the entity_addr_t encode/decode,
> > somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use
> > the new get_sockaddr_len() method for elen, though.
> >
> > Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the
> > transport version (IP version), which is already covered by the sa_family
> > field in the sockaddr.  I think that makes sense...
> >
> > A third commit would then add the entity_addrvec_t type, similar to what
> > is in that same commit.  It should also add the type to
> > test/encoding/types.h.
> >
> > Thanks!
> > sage
> >
> >
> >
> >>
> >> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
> >> > On Mon, 16 May 2016, Sage Weil wrote:
> >> >> Hi Junwang,
> >> >>
> >> >> On Mon, 16 May 2016, Junwang Zhao wrote:
> >> >> > Hi Sage, Haomai,
> >> >> >
> >> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
> >> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
> >> >> > that I am still working on, it has not been finished. I really need to check
> >> >> > with you whether I am doing it right, since it seems the errors are endless.
> >> >> >
> >> >> > There are some comments where I am not sure in the patch, like 'not sure'.
> >> >>
> >> >> I skimmed through this and it looks mostly right, but I see a few cases
> >> >> where features aren't needed, e.g. cls_refcount_get in
> >> >> cls_refcount_client.cc (there's no addr being encoded in
> >> >> cls_refcount_get_op, so no need to make the encoding featureful).
> >> >>
> >> >> > I didn't split this huge patch into small ones, I am not sure is that a
> >> >> > must, if yes, I will split it into small ones.
> >> >>
> >> >> The end result needs to be a series of small patches, but that doesn't
> >> >> have to happen right away.  I think it might be useful to do a few small
> >> >> sets of changes first just to show what the goal is, though.
> >> >>
> >> >> I will take the patch below and pull a few sample changes out so you can
> >> >> see.  Traveling at the moment, but I'll have something pushed today (that
> >> >> is also rebased on top of the latest wip-addr-cleanup branch).
> >> >
> >> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
> >> > there.  The last thing (I think) is updating msg/simple and msg/async.
> >> > These ones should explicitly encode with features 0 since the protocol is
> >> > defined in terms of the legacy encoding.
> >> >
> >> > sage
> >>
> >>
> 
> 

Re: [wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

Hi, Sage,

Check [1] when you got some time, sorry to bother :)

[1]https://github.com/zhjwpku/ceph/commit/8934f038fdf28156038770e601e4a27f7a684390

On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@newdream.net> wrote:
> On Thu, 19 May 2016, Junwang Zhao wrote:
>> Hi Sage, Haomai,
>> Please check [1] and give some feedback, I am not sure about
>> 52436ded943b8f567adb584c50b789431b788a85
>>
>> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>
> Great! I made several suggestions, and I have a question for everyone:
> what should we do if we have a "new" addr (say, for the v2 msgr protocol)
> and we need to encode it for a client that doesn't have the ADDR2 feature?
> Right now we are still stuffing the sockaddr_storage in their with
> whatever sockaddr data we have, but it is indistinguishable from a legacy
> address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or
> blank, except with a magic nonce (0xffffffff maybe) so that you can tell
> that it is non-blank but not terribly useful/usable?
>
> sage
>
>
>
>> > Thanks!
>>
>> On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@newdream.net> wrote:
>> > On Tue, 17 May 2016, Junwang Zhao wrote:
>> >> Hi Sage,
>> >>
>> >> Check this[1], we should I do next, entity_addrvec_t ?
>> >>
>> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>> >
>> > Looks good!  I opened the PR
>> >
>> >         https://github.com/ceph/ceph/pull/9184
>> >
>> > for review and test.
>> >
>> > The next step is just to make a new entity_addr_t encoding that is more
>> > compact and uses an ADDR2 feature.  One commit that defines the new
>> > feature, then one commit that changes the entity_addr_t encode/decode,
>> > somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use
>> > the new get_sockaddr_len() method for elen, though.
>> >
>> > Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the
>> > transport version (IP version), which is already covered by the sa_family
>> > field in the sockaddr.  I think that makes sense...
>> >
>> > A third commit would then add the entity_addrvec_t type, similar to what
>> > is in that same commit.  It should also add the type to
>> > test/encoding/types.h.
>> >
>> > Thanks!
>> > sage
>> >
>> >
>> >
>> >>
>> >> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
>> >> > On Mon, 16 May 2016, Sage Weil wrote:
>> >> >> Hi Junwang,
>> >> >>
>> >> >> On Mon, 16 May 2016, Junwang Zhao wrote:
>> >> >> > Hi Sage, Haomai,
>> >> >> >
>> >> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
>> >> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
>> >> >> > that I am still working on, it has not been finished. I really need to check
>> >> >> > with you whether I am doing it right, since it seems the errors are endless.
>> >> >> >
>> >> >> > There are some comments where I am not sure in the patch, like 'not sure'.
>> >> >>
>> >> >> I skimmed through this and it looks mostly right, but I see a few cases
>> >> >> where features aren't needed, e.g. cls_refcount_get in
>> >> >> cls_refcount_client.cc (there's no addr being encoded in
>> >> >> cls_refcount_get_op, so no need to make the encoding featureful).
>> >> >>
>> >> >> > I didn't split this huge patch into small ones, I am not sure is that a
>> >> >> > must, if yes, I will split it into small ones.
>> >> >>
>> >> >> The end result needs to be a series of small patches, but that doesn't
>> >> >> have to happen right away.  I think it might be useful to do a few small
>> >> >> sets of changes first just to show what the goal is, though.
>> >> >>
>> >> >> I will take the patch below and pull a few sample changes out so you can
>> >> >> see.  Traveling at the moment, but I'll have something pushed today (that
>> >> >> is also rebased on top of the latest wip-addr-cleanup branch).
>> >> >
>> >> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
>> >> > there.  The last thing (I think) is updating msg/simple and msg/async.
>> >> > These ones should explicitly encode with features 0 since the protocol is
>> >> > defined in terms of the legacy encoding.
>> >> >
>> >> > sage
>> >>
>> >>
>>
>>

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

On Thu, 19 May 2016, Junwang Zhao wrote:
> Hi, Sage,
> 
> Check [1] when you got some time, sorry to bother :)

That's what I'm here for!  :)

> [1]https://github.com/zhjwpku/ceph/commit/8934f038fdf28156038770e601e4a27f7a684390

It looks right to me, although I think we still want to follow up with a 
change to address my point below.

Thanks!
sage


> 
> On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@newdream.net> wrote:
> > On Thu, 19 May 2016, Junwang Zhao wrote:
> >> Hi Sage, Haomai,
> >> Please check [1] and give some feedback, I am not sure about
> >> 52436ded943b8f567adb584c50b789431b788a85
> >>
> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
> >
> > Great! I made several suggestions, and I have a question for everyone:
> > what should we do if we have a "new" addr (say, for the v2 msgr protocol)
> > and we need to encode it for a client that doesn't have the ADDR2 feature?
> > Right now we are still stuffing the sockaddr_storage in their with
> > whatever sockaddr data we have, but it is indistinguishable from a legacy
> > address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or
> > blank, except with a magic nonce (0xffffffff maybe) so that you can tell
> > that it is non-blank but not terribly useful/usable?
> >
> > sage
> >
> >
> >
> >> > Thanks!
> >>
> >> On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@newdream.net> wrote:
> >> > On Tue, 17 May 2016, Junwang Zhao wrote:
> >> >> Hi Sage,
> >> >>
> >> >> Check this[1], we should I do next, entity_addrvec_t ?
> >> >>
> >> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
> >> >
> >> > Looks good!  I opened the PR
> >> >
> >> >         https://github.com/ceph/ceph/pull/9184
> >> >
> >> > for review and test.
> >> >
> >> > The next step is just to make a new entity_addr_t encoding that is more
> >> > compact and uses an ADDR2 feature.  One commit that defines the new
> >> > feature, then one commit that changes the entity_addr_t encode/decode,
> >> > somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use
> >> > the new get_sockaddr_len() method for elen, though.
> >> >
> >> > Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the
> >> > transport version (IP version), which is already covered by the sa_family
> >> > field in the sockaddr.  I think that makes sense...
> >> >
> >> > A third commit would then add the entity_addrvec_t type, similar to what
> >> > is in that same commit.  It should also add the type to
> >> > test/encoding/types.h.
> >> >
> >> > Thanks!
> >> > sage
> >> >
> >> >
> >> >
> >> >>
> >> >> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
> >> >> > On Mon, 16 May 2016, Sage Weil wrote:
> >> >> >> Hi Junwang,
> >> >> >>
> >> >> >> On Mon, 16 May 2016, Junwang Zhao wrote:
> >> >> >> > Hi Sage, Haomai,
> >> >> >> >
> >> >> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
> >> >> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
> >> >> >> > that I am still working on, it has not been finished. I really need to check
> >> >> >> > with you whether I am doing it right, since it seems the errors are endless.
> >> >> >> >
> >> >> >> > There are some comments where I am not sure in the patch, like 'not sure'.
> >> >> >>
> >> >> >> I skimmed through this and it looks mostly right, but I see a few cases
> >> >> >> where features aren't needed, e.g. cls_refcount_get in
> >> >> >> cls_refcount_client.cc (there's no addr being encoded in
> >> >> >> cls_refcount_get_op, so no need to make the encoding featureful).
> >> >> >>
> >> >> >> > I didn't split this huge patch into small ones, I am not sure is that a
> >> >> >> > must, if yes, I will split it into small ones.
> >> >> >>
> >> >> >> The end result needs to be a series of small patches, but that doesn't
> >> >> >> have to happen right away.  I think it might be useful to do a few small
> >> >> >> sets of changes first just to show what the goal is, though.
> >> >> >>
> >> >> >> I will take the patch below and pull a few sample changes out so you can
> >> >> >> see.  Traveling at the moment, but I'll have something pushed today (that
> >> >> >> is also rebased on top of the latest wip-addr-cleanup branch).
> >> >> >
> >> >> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
> >> >> > there.  The last thing (I think) is updating msg/simple and msg/async.
> >> >> > These ones should explicitly encode with features 0 since the protocol is
> >> >> > defined in terms of the legacy encoding.
> >> >> >
> >> >> > sage
> >> >>
> >> >>
> >>
> >>
> 
> 

Re: [wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@newdream.net> wrote:
> On Thu, 19 May 2016, Junwang Zhao wrote:
>> Hi Sage, Haomai,
>> Please check [1] and give some feedback, I am not sure about
>> 52436ded943b8f567adb584c50b789431b788a85
>>
>> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>
> Great! I made several suggestions, and I have a question for everyone:
> what should we do if we have a "new" addr (say, for the v2 msgr protocol)
> and we need to encode it for a client that doesn't have the ADDR2 feature?
> Right now we are still stuffing the sockaddr_storage in their with
> whatever sockaddr data we have, but it is indistinguishable from a legacy
> address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or
> blank, except with a magic nonce (0xffffffff maybe) so that you can tell
> that it is non-blank but not terribly useful/usable?
>
I don't quite get this, can you explain this more specific? I now consider the
first encoded __u32 as the distinguish flag for the client to identify legacy
or not, a little bit confused :(

BTW, i have add entity_addrvec_t[1], you may want to have a look.

[1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

> sage
>
>
>
>> > Thanks!
>>
>> On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@newdream.net> wrote:
>> > On Tue, 17 May 2016, Junwang Zhao wrote:
>> >> Hi Sage,
>> >>
>> >> Check this[1], we should I do next, entity_addrvec_t ?
>> >>
>> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>> >
>> > Looks good!  I opened the PR
>> >
>> >         https://github.com/ceph/ceph/pull/9184
>> >
>> > for review and test.
>> >
>> > The next step is just to make a new entity_addr_t encoding that is more
>> > compact and uses an ADDR2 feature.  One commit that defines the new
>> > feature, then one commit that changes the entity_addr_t encode/decode,
>> > somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c.  I would use
>> > the new get_sockaddr_len() method for elen, though.
>> >
>> > Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the
>> > transport version (IP version), which is already covered by the sa_family
>> > field in the sockaddr.  I think that makes sense...
>> >
>> > A third commit would then add the entity_addrvec_t type, similar to what
>> > is in that same commit.  It should also add the type to
>> > test/encoding/types.h.
>> >
>> > Thanks!
>> > sage
>> >
>> >
>> >
>> >>
>> >> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@newdream.net> wrote:
>> >> > On Mon, 16 May 2016, Sage Weil wrote:
>> >> >> Hi Junwang,
>> >> >>
>> >> >> On Mon, 16 May 2016, Junwang Zhao wrote:
>> >> >> > Hi Sage, Haomai,
>> >> >> >
>> >> >> > I am working on the require-features-patch, I use 'make' to see the conflicts,
>> >> >> > and change the code to fit the required-feature-patch, [1] is a huge patch
>> >> >> > that I am still working on, it has not been finished. I really need to check
>> >> >> > with you whether I am doing it right, since it seems the errors are endless.
>> >> >> >
>> >> >> > There are some comments where I am not sure in the patch, like 'not sure'.
>> >> >>
>> >> >> I skimmed through this and it looks mostly right, but I see a few cases
>> >> >> where features aren't needed, e.g. cls_refcount_get in
>> >> >> cls_refcount_client.cc (there's no addr being encoded in
>> >> >> cls_refcount_get_op, so no need to make the encoding featureful).
>> >> >>
>> >> >> > I didn't split this huge patch into small ones, I am not sure is that a
>> >> >> > must, if yes, I will split it into small ones.
>> >> >>
>> >> >> The end result needs to be a series of small patches, but that doesn't
>> >> >> have to happen right away.  I think it might be useful to do a few small
>> >> >> sets of changes first just to show what the goal is, though.
>> >> >>
>> >> >> I will take the patch below and pull a few sample changes out so you can
>> >> >> see.  Traveling at the moment, but I'll have something pushed today (that
>> >> >> is also rebased on top of the latest wip-addr-cleanup branch).
>> >> >
>> >> > I went on a bit of a binge and have a wip-addr-work branch that is mostly
>> >> > there.  The last thing (I think) is updating msg/simple and msg/async.
>> >> > These ones should explicitly encode with features 0 since the protocol is
>> >> > defined in terms of the legacy encoding.
>> >> >
>> >> > sage
>> >>
>> >>
>>
>>

Re: [wip-addr-features] make sure I am doing the right thing

[Sage Weil]

On Fri, 20 May 2016, Junwang Zhao wrote:
> On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@newdream.net> wrote:
> > On Thu, 19 May 2016, Junwang Zhao wrote:
> >> Hi Sage, Haomai,
> >> Please check [1] and give some feedback, I am not sure about
> >> 52436ded943b8f567adb584c50b789431b788a85
> >>
> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
> >
> > Great! I made several suggestions, and I have a question for everyone:
> > what should we do if we have a "new" addr (say, for the v2 msgr protocol)
> > and we need to encode it for a client that doesn't have the ADDR2 feature?
> > Right now we are still stuffing the sockaddr_storage in their with
> > whatever sockaddr data we have, but it is indistinguishable from a legacy
> > address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or
> > blank, except with a magic nonce (0xffffffff maybe) so that you can tell
> > that it is non-blank but not terribly useful/usable?
>
> I don't quite get this, can you explain this more specific? I now 
> consider the first encoded __u32 as the distinguish flag for the client 
> to identify legacy or not, a little bit confused :(

Yeah, it is.  Look forward a bit to where we have a new, incompatible wire 
protocol, and TYPE_MSGR2 = 2 (to go with TYPE_LEGACY = 1).  How should we 
encode a msgr2 addr for a client that doesn't have the new ADDR2 feature?  
Currently, we'll set the legacy __u32 type = 0, and then encode same nonce 
and sockaddr, but that makes the msgr2 addr appear to be legacy addr to 
the legacy client.

Instead, I'm suggesting that if type != TYPE_LEGACY and the target doesn't 
have the ADDR2 feature, we encode type = 0, nonce = -1 (or some other 
poison placeholder), and an empty sockaddr.  That way the legacy code will 
see an addr that is not blank and also looks unusable (blank sockaddr, 
weird nonce).

> BTW, i have add entity_addrvec_t[1], you may want to have a look.
> 
> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work

A few comments, but looks good!

I will work on getting the discussion for the msgr2 protocol going next 
week, as we'll be needing that sooner rather than later.

Thanks!
sage

Re: [wip-addr-features] make sure I am doing the right thing

[Junwang Zhao]

On Fri, May 20, 2016 at 6:21 PM, Sage Weil <sage@newdream.net> wrote:
> On Fri, 20 May 2016, Junwang Zhao wrote:
>> On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@newdream.net> wrote:
>> > On Thu, 19 May 2016, Junwang Zhao wrote:
>> >> Hi Sage, Haomai,
>> >> Please check [1] and give some feedback, I am not sure about
>> >> 52436ded943b8f567adb584c50b789431b788a85
>> >>
>> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>> >
>> > Great! I made several suggestions, and I have a question for everyone:
>> > what should we do if we have a "new" addr (say, for the v2 msgr protocol)
>> > and we need to encode it for a client that doesn't have the ADDR2 feature?
>> > Right now we are still stuffing the sockaddr_storage in their with
>> > whatever sockaddr data we have, but it is indistinguishable from a legacy
>> > address.  Perhaps it sould encode as blank?  (e.g., entity_addr_t())? Or
>> > blank, except with a magic nonce (0xffffffff maybe) so that you can tell
>> > that it is non-blank but not terribly useful/usable?
>>
>> I don't quite get this, can you explain this more specific? I now
>> consider the first encoded __u32 as the distinguish flag for the client
>> to identify legacy or not, a little bit confused :(
>
> Yeah, it is.  Look forward a bit to where we have a new, incompatible wire
> protocol, and TYPE_MSGR2 = 2 (to go with TYPE_LEGACY = 1).  How should we
> encode a msgr2 addr for a client that doesn't have the new ADDR2 feature?
> Currently, we'll set the legacy __u32 type = 0, and then encode same nonce
> and sockaddr, but that makes the msgr2 addr appear to be legacy addr to
> the legacy client.
>
> Instead, I'm suggesting that if type != TYPE_LEGACY and the target doesn't
> have the ADDR2 feature, we encode type = 0, nonce = -1 (or some other
> poison placeholder), and an empty sockaddr.  That way the legacy code will
> see an addr that is not blank and also looks unusable (blank sockaddr,
> weird nonce).
>

Thanks, understand now :)

>> BTW, i have add entity_addrvec_t[1], you may want to have a look.
>>
>> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work
>
> A few comments, but looks good!

Updated, I add a entity_addr_t::decode_legacy_addr_after_marker()
helper to eliminate the duplication, have a look when you got some time.

>
> I will work on getting the discussion for the msgr2 protocol going next
> week, as we'll be needing that sooner rather than later.
>
Look forward to that!

Thanks!

> Thanks!
> sage