Set tenant_cache_seconds globally

[awaazde]

Set tenant_cache_seconds globally

[maciej-gol]

Looks nice, I would just add a test that actually checks for this behavior. Like an app with cache seconds and two tasks: one with overriden seconds, the other not. pon., 4 maj 2020, 16:15 użytkownik Awaaz.De <[email protected]> napisał:

…

------------------------------ You can view, comment on, or merge this pull request online at: #38 Commit Summary - Issue #28: allow tenant_cache_seconds to be set globally File Changes - *M* tenant_schemas_celery/task.py <https://github.com/maciej-gol/tenant-schemas-celery/pull/38/files#diff-114e76a57a30739642f19fed4a7b3bc2> (5) Patch Links: - https://github.com/maciej-gol/tenant-schemas-celery/pull/38.patch - https://github.com/maciej-gol/tenant-schemas-celery/pull/38.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#38>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOGQTLX4OW3FRM6EV3YZ43RP3EZZANCNFSM4MYZLB5Q> .

[awaazde]

@maciej-gol I updated the code to what should be correct.

I'm stuck on test cases bc I can't get the test env to run. Can you see these errors and suggest how to get tests to work?

https://gist.github.com/awaazde/25af5604301f3112441983395d7c1f62
https://gist.github.com/awaazde/a5cfd3d94fd4383739fa82e17a1e61e5

[maciej-gol]

Just run the ./run-tests script. Requires docker-compose. It will print out some error tracebacks because of missing model objects, but these are expected errors. As long as pytest claims 100% success, then it's fine.

[awaazde]

Just run the ./run-tests script. Requires docker-compose. It will print out some error tracebacks because of missing model objects, but these are expected errors. As long as pytest claims 100% success, then it's fine.

Can you confirm this is output is expected? I don't think tests passed but not clear.

[awaazde]

@maciej-gol looking at the implementation of SimpleCache, the storage here could get pretty big in memory over time depending on expiry length and how long the worker is alive.

Do you think it's a concern? Maybe the storage should clear expired entries?

[maciej-gol]

Can you confirm this is output is expected? I don't think tests passed but not clear.

Yes, it died. You can see it both in error messages before pytest was ran, and during pytest itself - some tests are reported as errors E. Can you rebase on top of newest master and rerun the script? It should declutter the output significantly so that we can debug it easier.

@maciej-gol looking at the implementation of SimpleCache, the storage here could get pretty big in memory over time depending on expiry length and how long the worker is alive.

Do you think it's a concern? Maybe the storage should clear expired entries?

I think it's a general good practice to setup celery so that it kills it's workers after a number of requests handled. CPython is known of being not so eager to let OS reclaim free memory.

Other than that, will look into existing LRU solutions so that I don't need to reimplement it.

[maciej-gol]

@awaazde there is an interesting package https://github.com/tkem/cachetools that I could use as TTLCache, I will need to sunset per-task cache setting in favor of the global one before proceeding.

[awaazde]

Can you confirm this is output is expected? I don't think tests passed but not clear.

Yes, it died. You can see it both in error messages before pytest was ran, and during pytest itself - some tests are reported as errors E. Can you rebase on top of newest master and rerun the script? It should declutter the output significantly so that we can debug it easier.

Reran and result here. This looks much better can you confirm this is baseline passing? I will now add a test case

[maciej-gol]

...
app_1       | =========================== 8 passed in 3.20 seconds ===========================
tenant-schemas-celery_app_1 exited with code 0

Worked like a charm!

[awaazde]

@maciej-gol this should be good to go. Added test coverage for the global var.

I spent several hours trying to track down why the setting was not being loaded by Celery config. I had to force load the settings in the test case, which ideally shouldn't be required. However I believe the fix is setting namespace="CELERY" here in test_app.py, but that seems to be failing because this somehow breaks the rabbitmq broker login.

I couldn't quite figure it out but if you can we may be able to remove this line

[maciej-gol]

If you would like me to help with the MR, I need access to your fork.

[awaazde]

If you would like me to help with the MR, I need access to your fork.

Gave you access let me know if you're not able to.

[awaazde]

@maciej-gol I've addressed all the comments above. My main concern is the versioning of this package seems off. You have labeled the pip package version 0.3.0, but master branch has python_requires>=3.6. According to the docs though, 0.x version is supposed to support Python 2.x.

So I believe that we need to either:

• revert the line above in setup.py on master (I've done it on this fork currently)
• create a branch off of tag 0.3.0, merge/backport these changes there, and either label that branch as v0.3.1 and/or tag it as v0.3.1. I've setup a separate PR Set tenant_cache_seconds globally (v.0.3.0) #42 for this, but this option doesn't seem ideal.

Let me know your thoughts.

[maciej-gol]

I will create the 0.x branch and back port the changes, no worries. czw., 21 maj 2020, 08:43 użytkownik Awaaz.De <[email protected]> napisał:

…

@maciej-gol <https://github.com/maciej-gol> I've addressed all the comments above. My main concern is the versioning of this package seems off. You have labeled the pip package version 0.3.0, but master branch has python_requires>=3.6 <https://github.com/maciej-gol/tenant-schemas-celery/blob/dd02ba4f190f6fca3af908373fff8e59f9c78a1c/setup.py#L30> . According to the docs <https://github.com/maciej-gol/tenant-schemas-celery#python-compatibility> though, 0.x version is supposed to support Python 2.x. So I believe that we need to either: - revert the line above in setup.py on master (I've done it on this fork currently) - create a branch off of tag 0.3.0, merge these changes there, and either label that branch as v0.3.1 and/or tag it as v0.3.1. I've setup a separate PR #42 <#42> for this, but this option doesn't seem ideal. Let me know your thoughts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOGQTKDZHGBQZQLO5CIDPDRSTEP3ANCNFSM4MYZLB5Q> .

[awaazde]

I will create the 0.x branch and back port the changes, no worries. czw., 21 maj 2020, 08:43 użytkownik Awaaz.De [email protected] napisał:

OK also closed PR #42 as it's not relevant

[awaazde]

@maciej-gol, do you have an estimate on when you'll be able to merge and backport this enhancement for Python < 3.x?

We want to include the updated package in our next release, based on your timeline we can plan our release accordingly.

Thank you!

[maciej-gol]

I will merge it, then, and correct the tests later

[maciej-gol]

Released as 0.4.0.

[awaazde]

@maciej-gol master branch is currently tagged with v0.3.0. Should that be bumped or changed to avoid confusion?

Also may need to tweak the docs slightly to mention the global setting introduced in 0.4.0

[maciej-gol]

The master branch will be bumped to 1.0 at some point. There has been no release on master yet, thus old version. Will update 0.4.0 docs. czw., 28 maj 2020, 15:28 użytkownik Awaaz.De <[email protected]> napisał:

…

@maciej-gol <https://github.com/maciej-gol> master branch is currently tagged with v0.3.0. Should that be bumped or changed to avoid confusion? Also may need to tweak the docs slightly to mention the global setting introduced in 0.4.0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOGQTLZDCJIAKETS4MVIPLRTZRH3ANCNFSM4MYZLB5Q> .