Chaffelson / nipyapi

A convenient Python wrapper for Apache NiFi
Other
243 stars 76 forks source link

Added gzip as default request header #277

Closed rsaggino closed 2 years ago

rsaggino commented 3 years ago

Fixes #273 by adding gzip headers to the api client.

Tested on a small dev cluster, nipyapi 0.16.2 and NiFi 1.11. It seems like there were additional improvements on NiFi/nipyapi that reduced the impact of this patch, this is still necessary on larger clusters and while making complex automations.

Before:

In []: %%time
   ...: import nipyapi
   ...: a=nipyapi.canvas.list_all_processors()
   ...:
   ...:
CPU times: user 22.3 s, sys: 3.46 s, total: 25.8 s
Wall time: 1min 27s

In []: %%time
   ...: import nipyapi
   ...: a=nipyapi.canvas.list_all_controllers()
   ...:
   ...:
CPU times: user 12.1 s, sys: 2.24 s, total: 14.3 s
Wall time: 35.7 s

After:

In []: %%time
   ...: import nipyapi
   ...: a=nipyapi.canvas.list_all_processors()
   ...:
   ...:
CPU times: user 19.8 s, sys: 1.11 s, total: 20.9 s
Wall time: 1min 1s

In []: %%time
   ...: import nipyapi
   ...: a=nipyapi.canvas.list_all_controllers()
   ...:
   ...:
CPU times: user 11.4 s, sys: 481 ms, total: 11.9 s
Wall time: 19.6 s
coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.3%) to 69.11% when pulling 64e2a9d6b4a9566ce1f31f1e74bd9d0ad9cccb9e on rsaggino:gzip_nifi_api into 292aa1e8bc1c4d080d7b6acdf0cedfb4e77a1f03 on Chaffelson:main.

ottobackwards commented 3 years ago

https://github.com/swagger-api/swagger-codegen/issues/10085

So, this is a problem with the code gen. I'm not sure we would want to change this file itself, since it is generated.

Normally for issues such as this, I would apply a patch post generation. Thoughts @Chaffelson ?

ottobackwards commented 3 years ago

Note, that given url lib support for gzip, this is the right fix for the generated code It is just how it is managed.

rsaggino commented 3 years ago

Normally for issues such as this, I would apply a patch post generation. Thoughts @Chaffelson ?

That would be the best option (aside from fixing this upstream). Is the post-gen scripted somewhere or completely manual?

ottobackwards commented 3 years ago

https://github.com/Chaffelson/nipyapi/tree/main/resources/client_gen

Chaffelson commented 3 years ago

The client is primarily generated using mustache templates, so if this was added to the template then all future clients would pick it up. https://github.com/Chaffelson/nipyapi/blob/292aa1e8bc1c4d080d7b6acdf0cedfb4e77a1f03/resources/client_gen/swagger_templates/api_client.mustache#L58

rsaggino commented 3 years ago

The client is primarily generated using mustache templates, so if this was added to the template then all future clients would pick it up.

https://github.com/Chaffelson/nipyapi/blob/292aa1e8bc1c4d080d7b6acdf0cedfb4e77a1f03/resources/client_gen/swagger_templates/api_client.mustache#L58

This is an easy fix but could get lost in future updates of the templates. Any preference?

Chaffelson commented 3 years ago

They are my templates, not upstream, so it won't be lost

On Fri, 11 Jun 2021, 15:57 rsaggino, @.***> wrote:

The client is primarily generated using mustache templates, so if this was added to the template then all future clients would pick it up.

https://github.com/Chaffelson/nipyapi/blob/292aa1e8bc1c4d080d7b6acdf0cedfb4e77a1f03/resources/client_gen/swagger_templates/api_client.mustache#L58

This is an easy fix but could get lost in future updates of the templates. Any preference?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Chaffelson/nipyapi/pull/277#issuecomment-859640710, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZAZOBJY3I7VLFKYZZUTFLTSIP4RANCNFSM46A4TE7A .

ottobackwards commented 3 years ago

@Chaffelson maybe you can list out explicitly what we would want to do?

ottobackwards commented 3 years ago

ie:

rsaggino commented 3 years ago

Applied the change also to the mustache template. Given the idea to use Bravado in the future, we could leave the generate script aside.

rsaggino commented 3 years ago

@Chaffelson can we proceed with this?

ottobackwards commented 3 years ago

I'm +1 on this, pending whatever testing @Chaffelson does for these kinds of changes

ottobackwards commented 3 years ago

fwiw :)

Chaffelson commented 3 years ago

Yeah this is the way to do it, I was going to cut a release this week but am dealing with unexpected covid isolation so it'll have to be next week sorry.

Chaffelson commented 3 years ago

Managed to get some testing on this done today and it's looking good, I'll be rolling up the new client for 1.13 in the next week and including this in it