Re: [Twisted-Python] revert / rework of "AMPv2" change?

[Glyph]

> On Oct 21, 2020, at 10:34 AM, Jonathan Bastien-Filiatrault  
> wrote:
> 
> On 2020-10-20 04 h 57, adi at roiban.ro (Adi Roiban) wrote:
>> Just my 2 cents
>> I think that whether this should be reverted or reworked should be between
>> the person raising this issue (Glyph) and the author of the PR (Jonathan).
> Glyph has asked me to submit the type annotation work on AMP, I will do
> this. I also plan on resubmitting a reworked patch that addresses the
> issues that glyph raised.
>> 
>> I see that Jonathan has responded to the feedback inside the merged PR.I
>> hope it can be reworked.
>> 
>> Since AMP v2 is not standardized maybe is ok to have it as a separate
>> experimental top level name.
>> 
>> I have never used AMP ...I tried to use it via ampoule but ended up using
>> the stdlib process pool as I was not able see any performance difference.
>> 
>> Since `amp` is used by `trial -j`  we need to keep it inside twisted.
>> 
>> But since AMP v2 is experimental, I think that is best to have it as a
>> separate txamp2 project.
>> In this way, the twisted AMP v2 project can follow its own rules.
>> Once AMP v2 is "mature" it can be moved to core twisted
>> 
>> So my take is to revert it and move it to twisted/txamp2.
> 
> I have no issue with changing the name or if this patch lives as a
> project besides twisted. I might even rework it into something more
> forward-compatible as suggested by glyph. I think netstrings are a
> decent option as it would make long value handling easier since values
> would always be contiguous without "continuation markers" interspersed.
> The main thing I dislike about netstrings are the variable-length ASCII
> numbers, but that is not a big complexity.

I originally didn't like this either, but since I designed AMP I've come around:

The thing about variable-length ASCII numbers is that they give you a lot of 
leeway to error out in case the protocol is not being respected.  With packed 
binary values, any 2-byte sequence is a valid length, so if something opens a 
socket and says "EHLO", it starts waiting for 17734 more bytes which it will 
never receive.  By contrast, netstrings have built-in redudancy; for a very low 
overhead (most length prefixes end up being 2-3 bytes rather than 2) you get 
the ability to immediately error out with a clear exception as soon as you see 
"E".  Anyone implementing this in a static context where they don't want heap 
allocations for the length prefix itself should not allocate arbitrary data, 
but rather, allocate a very small fixed buffer (6 bytes will get you 
considerably beyond the built-in limitations of AMP already) and again, 
immediately error out if that is exceeded.  Similarly the trailing comma 
provides an in-band sanity check where buggy applications will promptly crash 
rather than bleeding into the next message.

> Could the next AMP version
> use values framed using 32 bit or 64 bit integers ?

I'd really rather go with netstrings for all the reasons listed above.

> Transmitting long-ish values is a real use case and whatever extension
> we do must retain the dead-simple, easy to implement nature of the
> current AMP protocol. I now realise that I somewhat hijacked the
> "streaming values" bug (2529). IMHO, streaming should be handled at the
> application level, but I digress.

Yeah, that bug is more about making it easy to do the application-level stuff 
than implementing something implicit within the protocol.

>> 
>> 
>> BTW this is also my suggestion for the PR trying to introduce support for
>> SMB have twisted SMB implementation in a separate project.
>> 
> All the best,
> 
> Jonathan

Same to you.  Thanks for your contributions, and sorry for the slow pace of 
communication here; 2020 is a heck of a year.

-g___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] revert / rework of "AMPv2" change?

[Jonathan Bastien-Filiatrault]

On 2020-10-20 04 h 57, adi at roiban.ro (Adi Roiban) wrote:
> Just my 2 cents
> I think that whether this should be reverted or reworked should be between
> the person raising this issue (Glyph) and the author of the PR (Jonathan).
Glyph has asked me to submit the type annotation work on AMP, I will do
this. I also plan on resubmitting a reworked patch that addresses the
issues that glyph raised.
>
> I see that Jonathan has responded to the feedback inside the merged PR.I
> hope it can be reworked.
>
> Since AMP v2 is not standardized maybe is ok to have it as a separate
> experimental top level name.
>
> I have never used AMP ...I tried to use it via ampoule but ended up using
> the stdlib process pool as I was not able see any performance difference.
>
> Since `amp` is used by `trial -j`  we need to keep it inside twisted.
>
> But since AMP v2 is experimental, I think that is best to have it as a
> separate txamp2 project.
> In this way, the twisted AMP v2 project can follow its own rules.
> Once AMP v2 is "mature" it can be moved to core twisted
>
> So my take is to revert it and move it to twisted/txamp2.


I have no issue with changing the name or if this patch lives as a
project besides twisted. I might even rework it into something more
forward-compatible as suggested by glyph. I think netstrings are a
decent option as it would make long value handling easier since values
would always be contiguous without "continuation markers" interspersed.
The main thing I dislike about netstrings are the variable-length ASCII
numbers, but that is not a big complexity. Could the next AMP version
use values framed using 32 bit or 64 bit integers ?


Transmitting long-ish values is a real use case and whatever extension
we do must retain the dead-simple, easy to implement nature of the
current AMP protocol. I now realise that I somewhat hijacked the
"streaming values" bug (2529). IMHO, streaming should be handled at the
application level, but I digress.

>
> 
>
> BTW this is also my suggestion for the PR trying to introduce support for
> SMB have twisted SMB implementation in a separate project.
>
All the best,

Jonathan

___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Re: [Twisted-Python] revert / rework of "AMPv2" change?

[Adi Roiban]

On Mon, 19 Oct 2020 at 18:18, Glyph  wrote:

> I just glanced at https://github.com/twisted/twisted/pull/1417 after it
> was merged and noticed a few problems:
>
> - It exposes a new top-level name rather than setting the protocol version
> as a parameter
> - The NEWS files are malformed which is going to lead to some confusing
> duplication in the changelog
> - AMPv2 itself does not appear to be "standardized" - the page at
> https://amp-protocol.net/conversations_v2.html appears to be a
> preliminary suggestion for how long values might be handled rather than
> something broadly implemented; for one thing, I'd never heard of it before
> and for another the site itself doesn't link the document. This would be
> fine as a first draft, but I think it needs to be given some more
> revisions, given that (for example) there are a number of
> backwards-compatible ways to do this.
> - The layering is wrong because it puts the protocol-parsing into a leaf
> class in the hierarchy, when the parsing logic was deliberately isolated to
> a lower level to facilitate different framing mechanisms.
> - As JP pointed out, the tests have a potential bug where they can leak
> errors between cases.
>
> I don't normally like to revert folks' work once it's been reviewed and
> accepted, particularly when there's no process violation (broken CI, lack
> of code coverage etc), but I'm particularly concerned about lending
> Twisted's imprimatur to this protocol extension as a whole new version
> without much more context on who is implementing it and what other options
> were considered, particularly when I personally (nominally the inventor of
> AMP) don't like the direction it has taken :).
>
> What do other folks think?  Anyone else have more of a finger on the pulse
> of where the "v2" conversation has been happening?
>
>
Just my 2 cents

I think that whether this should be reverted or reworked should be between
the person raising this issue (Glyph) and the author of the PR (Jonathan).

I see that Jonathan has responded to the feedback inside the merged PR.I
hope it can be reworked.

Since AMP v2 is not standardized maybe is ok to have it as a separate
experimental top level name.

I have never used AMP ...I tried to use it via ampoule but ended up using
the stdlib process pool as I was not able see any performance difference.

Since `amp` is used by `trial -j`  we need to keep it inside twisted.

But since AMP v2 is experimental, I think that is best to have it as a
separate txamp2 project.
In this way, the twisted AMP v2 project can follow its own rules.
Once AMP v2 is "mature" it can be moved to core twisted

So my take is to revert it and move it to twisted/txamp2.



BTW this is also my suggestion for the PR trying to introduce support for
SMB have twisted SMB implementation in a separate project.

-- 
Adi Roiban
___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Twisted-Python] revert / rework of "AMPv2" change?

[Glyph]

I just glanced at https://github.com/twisted/twisted/pull/1417 
 after it was merged and noticed 
a few problems:

- It exposes a new top-level name rather than setting the protocol version as a 
parameter
- The NEWS files are malformed which is going to lead to some confusing 
duplication in the changelog
- AMPv2 itself does not appear to be "standardized" - the page at 
https://amp-protocol.net/conversations_v2.html 
 appears to be a preliminary 
suggestion for how long values might be handled rather than something broadly 
implemented; for one thing, I'd never heard of it before and for another the 
site itself doesn't link the document. This would be fine as a first draft, but 
I think it needs to be given some more revisions, given that (for example) 
there are a number of backwards-compatible ways to do this.
- The layering is wrong because it puts the protocol-parsing into a leaf class 
in the hierarchy, when the parsing logic was deliberately isolated to a lower 
level to facilitate different framing mechanisms.
- As JP pointed out, the tests have a potential bug where they can leak errors 
between cases.

I don't normally like to revert folks' work once it's been reviewed and 
accepted, particularly when there's no process violation (broken CI, lack of 
code coverage etc), but I'm particularly concerned about lending Twisted's 
imprimatur to this protocol extension as a whole new version without much more 
context on who is implementing it and what other options were considered, 
particularly when I personally (nominally the inventor of AMP) don't like the 
direction it has taken :).

What do other folks think?  Anyone else have more of a finger on the pulse of 
where the "v2" conversation has been happening?

-glyph

___
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python