Adding test for HTTP/2 draining

[zizhong]

@maskit @masaori335 can you please review?
@bryancall @zwoop I copied the test structure from hyper example code in MIT license. is it okay for us to use?

[maskit]

[approve ci]

From my understanding, copyright statement has to be kept even if it was MIT license, if you copied the example code.

[zizhong]

@maskit Do I also need to add the Apache license?

[maskit]

Yes, probably we should do that. Code that licensed under MIT should be able to sublicense under Apache license.

[maskit]

[approve ci]

[maskit]

It looks a bit confusing. Maybe someone read it as dual license, which users can choose one from them.

You should clarify that the original code was distributed under MIT license with the copyright notice.

@zwoop @bryancall Do you have good examples or references?

[maskit]

The test itself looks good to me. Only concern is availability and compatibility of pkill command. If possible, it should be done by calling some function provided by Python.

[maskit]

[approve ci]

[maskit]

Please resolve the license header issue. RAT check recognized the file as MIT, which is not what we expected.

MIT ./tests/gold_tests/h2/h2drain.py

I think an acceptable format is like this.

<Apache license>

The original code was distributed under MIT license below.

<MIT License with the original copyright notice>

[zizhong]

@maskit updated! can you review it again? Thanks!

[maskit]

[approve ci]

[maskit]

[approve ci autest]

[zizhong]

Not sure why the autest failed on this one. Works locally every time. I need to look into it more.

[bryancall]

[approve ci autest]

RAT check passed

[bryancall]

[approve ci autest]

[maskit]

The format still looks a bit weird to me.

The original code was under MIT but now we are trying to relicense a modified version under AL2.
Without this information, it looks like we just concatenated two source files licensed under different licenses into one file (but actually it's not since some of import lines came from the original code).

I'd like to hear opinions from others.

[bryancall]

[approve ci debian]

[dragon512]

All these CopyAs can be more terse with just a
ts.Setup.Copy('h2client.py')

[zwoop]

[approve ci autest]

[bryancall]

@zizhong Can you fix the merge conflict?

@zizhong @dragon512 Is this ready to land after the merge has been fixed?

[zizhong]

Let me check on the conflicts.
@bryancall Any comment on the dual licenses?

[zizhong]

I updated my branch and the conflict is gone. However, the test is not working for now.
I'll submit a small fix soon.

[bryancall]

[approve ci autest]

[zwoop]

@zizhong What do we want to do with this PR ?

[zizhong]

@zwoop I'll update this and #3693 today.

[zizhong]

[approve ci autest]

[zizhong]

  File "h2drain.py", line 26, in <module>
    import twisted.internet._sslverify as v
ModuleNotFoundError: No module named 'twisted'

, but I added twisted to bootstrap.py.
@zwoop @dragon512 what's the right way to tell the Autest env installing new python libs?

[bryancall]

[approve ci autest]

[bryancall]

@zizhong The PR is failing in autest, I would like to get it merged or close it since it hasn't been updated in awhile.

[zizhong]

@bryancall these tests work perfectly locally, where I manually installed twisted. To get it passing the autests, I would like to get some help to figure out what the correct way to install a python dep in autest. Do you happen to know the answer? Thanks!
cc @zwoop @dragon512 @maskit

[zwoop]

Twisted is quite a beast, and I'd much prefer not to incorporate it into our autest dependencies. That much said, that's up to @dragon512 to decide.

[dragon512]

I have no issue with using twisted. It is network based python framework used by lots of people. I think the question for me at the moment is what is it being used for here and can this be done with the other tools we already have. @macisasandwich Can you look and see if this could be done with our existing tools you have been putting effort into?

[x-jesse-zhang]

Right now we've got no H2 capabilities with microserver and it will probably be an undefined amount of time before I can get around to adding H2 and draining to the tools that we use. So we should add this one in.

[zwoop]

I feel reasonably strongly that we should not introduce Twisted into our system. Things are already complicated, and Twisted amplifies that by another magnitude.

[zwoop]

Also, not sure why you would need H2 capabilities in the micro server ? This is acting as an H2 client (I can only assume, I can not decipher the code), and we have no H2 to origin support.

Can't we implement this as either just using curl commands, or perhaps some simpler, basic Python H2 client, such as https://hyper.readthedocs.io/en/latest/, which we already require anyways ?

[zizhong]

The twisted code is needed for detecting the goaway frame, which I don't think cURL or hyper gives us. Leif Hedstrom <notifications@github.com> 于2019年3月28日周四 上午10:03写道：

…

Also, not sure why you would need H2 capabilities in the micro server ? This is acting as an H2 client (I can only assume, I can not decipher the code), and we have no H2 to origin support already. Can't we implement this as either just using curl commands, or perhaps some simpler, basic Python H2 client, such as https://hyper.readthedocs.io/en/latest/, which we already require anyways ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2918 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADdQjGF4Hbpg4v7vM8QFYr2zNJum1BaAks5vbPX5gaJpZM4Q_57x> .