<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 21, 2016 at 3:16 PM, James E. Blair <span dir="ltr"><<a href="mailto:corvus@inaugust.com" target="_blank">corvus@inaugust.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">Morgan Fainberg <<a href="mailto:morgan.fainberg@gmail.com">morgan.fainberg@gmail.com</a>> writes:<br>
<br>
> As I have been converting Zuul and NodePool to python3, I have had to do a<br>
> bunch of changes around encode() and decode() of strings since gear is<br>
> (properly) an implementation of a protocol that requires binary data<br>
> (rather than text_strings).<br>
><br>
> What this has highlighted is that gear should be made a bit more friendly<br>
> to use in the python world. We already explicitly assume a utf-8 encoding<br>
> for when things are turned into "binary" when crafting the job object in<br>
> certain cases [1].  I have discussed this with Jim Blair, and we both agree<br>
> that the ability to still reference attributes such as "<a href="http://job.name" rel="noreferrer" target="_blank">job.name</a>" (in a<br>
> simple manner that is straight forward), is important.<br>
<br>
</span>Thanks for this email -- with this and a little browsing of the gear<br>
code, I think I've absorbed most of the context and am able to process<br>
this.  :)<br>
<span class=""><br>
> Here is the outline of the change I'm proposing:<br>
><br>
> * The main consumable part of gear (public classes) will convert the<br>
> "string" data we assign ( name[2], unique[3]) into utf-8-encoded bytes via<br>
> @property, @property.setter, and @property.getter for public consumption.<br>
<br>
</span>This seems good.  I think we can make the assertion that in the API<br>
these things should always be utf-8 encoded strings, and do what's<br>
needed behind the scenes to encode/decode to bytes.<br>
<br>
I think this is the bulk of what we need to do to make gear<br>
user-friendly.<br>
<span class=""><br></span></blockquote><div><br></div><div>++ This is where I am going to focus.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">
> * Arguments are explicitly supposed to be a binary_blob [4]. I am unsure if<br>
> this should also be automatically converted *or* if it should be the<br>
> inverse, .arguments and .arguments_string ?<br>
<br>
</span>This is the thing we can't handle automatically.  At least, we can on<br>
the input side (if the user passes in a string, we could auto-encode but<br>
leave bytes alone -- but on the output side since the protocol is<br>
un-typed, we wouldn't know what to do).  So maybe this is a place where<br>
we should force the user to encode/decode.<br>
<br>
Looking at the gear code, I think that was the intent in a lot of<br>
places, but I think we may have gotten ahead of ourselves and written<br>
code assuming that isinstance('string', bytes) is False (which is the<br>
case in python3, but not in python2):<br>
<br>
  <a href="https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L589" rel="noreferrer" target="_blank">https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L589</a><br>
<br>
That line of code explains a lot to me.<br>
<br>
I think we should stick close to the idea that arguments are binary<br>
blobs.<br>
<br>
>From that, it seems to follow that reading job.arguments should get you<br>
a bytes object.<br>
<br>
On the one hand, I don't think it's unreasonable to say that users must<br>
encode/decode on that boundary.<br>
<br>
On the other hand, I think we could easily have a job.arguments_string<br>
property that decodes that from utf-8 for convenience, so why not?  I<br>
think I'm in favor of this.<br>
<br>
If we do that, then I think the question is: do we have gear<br>
automatically encode job arguments (in the Job constructor) if the user<br>
passes in a string?  I tend to favor this as well because it should<br>
almost always do the right thing in both python2 and python3 without<br>
inhibiting usage for either strings or binary data.  Does that sound<br>
reasonable?<br>
<span class=""><br></span></blockquote><div><br></div><div>I've spent a lot of time thinking about this, I'm going to say that I agree, but I</div><div>feel like the _string property will be mostly unused. I want to see how things look</div><div>before we add it in. If it makes things a lot better we can always add it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">
> * Internally gear will reference the encoded bits via the underlying<br>
> <name>_binary form, which will allow direct access to a non-"string" form<br>
> of the data itself in the case that there is interesting things that need<br>
> to be handled via binary packing (for example) instead of "stringified"<br>
> versions.<br>
<br>
</span>Sounds good.  Maybe we should call it "<name>_bytes" though for clarity?<br>
/bikeshed<br>
<span class=""><br></span></blockquote><div><br></div><div>HAH! Sure. But as long as the bikeshed is cherenkov radiation blue. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="">
> * For compatibility the main @property.setter will handle either<br>
> binary_type or string_type (so we don't break everyone).<br>
<br>
</span>Most of these are set in the constructor -- having a setter do that<br>
should make the constructor do the right thing, but in case there are<br>
cases where we need to remove type validation, I think it's worth saying<br>
that our intent is to have the constructor handle both forms as well for<br>
the same reason.<br>
<span class=""><br>
> * The "<name>_binary" will enforce that the data with be a binary_type<br>
> only.<br>
><br>
><br>
> I think this can be done in a single release of gear with minimal impact to<br>
> those using it. For what it is worth, it is unlikely that anyone has used<br>
> gear extensively in python3 as of yet because of recent bug fixes that<br>
> addressed py2->py3 compat issues around dict.values() and similar list() -><br>
> iter() changes.<br>
<br>
</span>Agreed.<br>
<span class=""><br>
> See the one question in the above proposal for "arguments".<br>
><br>
> [1]<br>
> <a href="https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L86" rel="noreferrer" target="_blank">https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L86</a><br>
> [2]<br>
> <a href="https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2054" rel="noreferrer" target="_blank">https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2054</a><br>
> [3]<br>
> <a href="https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2058" rel="noreferrer" target="_blank">https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2058</a><br>
> [4]<br>
> <a href="https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2056" rel="noreferrer" target="_blank">https://github.com/openstack-infra/gear/blob/59d29104cb9c370e49b44f313e568bd43b172e1b/gear/__init__.py#L2056</a><br>
><br>
> Thanks,<br>
> --Morgan<br>
</span>> _______________________________________________<br>
> OpenStack-Infra mailing list<br>
> <a href="mailto:OpenStack-Infra@lists.openstack.org">OpenStack-Infra@lists.openstack.org</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra</a><br>
<br>
Thanks again!<br>
<br>
-Jim<br>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Cheers,</div><div class="gmail_extra">--Morgan</div></div>