-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add own service for memcached #8818
Add own service for memcached #8818
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @EHJ-52n on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8818 +/- ##
=======================================
Coverage 61.95% 61.95%
=======================================
Files 867 867
Lines 51577 51577
Branches 6467 6467
=======================================
+ Hits 31954 31956 +2
+ Misses 18084 18081 -3
- Partials 1539 1540 +1 |
Welcome @EHJ-52n
I see this is a change that shall not be visible to end-users but nonetheless, I would like to have a proper improvement description as an issue. @afabiani @giohappy Do you confirm my demand, or do you guys think this can be a PR without issue? Same for #8817 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I know this is a bit confusing, but this option MEMCACHED_LOCATION
is not currently driving the memcached
lock, this one setting the frontend
cache instead
MEMCACHED_ENABLED = ast.literal_eval(os.getenv('MEMCACHED_ENABLED', 'False'))
MEMCACHED_BACKEND = os.getenv('MEMCACHED_BACKEND', 'django.core.cache.backends.memcached.PyMemcacheCache')
MEMCACHED_LOCATION = os.getenv('MEMCACHED_LOCATION', '127.0.0.1:11211')
MEMCACHED_LOCK_EXPIRE = int(os.getenv('MEMCACHED_LOCK_EXPIRE', 3600))
MEMCACHED_LOCK_TIMEOUT = int(os.getenv('MEMCACHED_LOCK_TIMEOUT', 10))
if MEMCACHED_ENABLED:
CACHES['default'] = {
'BACKEND': MEMCACHED_BACKEND,
'LOCATION': MEMCACHED_LOCATION,
}
The celery tasks
will use only the MEMCACHED_LOCATION
variable instead
try:
import pylibmc
import sherlock
from sherlock import MCLock as Lock
sherlock.configure(
expire=settings.MEMCACHED_LOCK_EXPIRE,
timeout=settings.MEMCACHED_LOCK_TIMEOUT)
memcache_client = pylibmc.Client(
[settings.MEMCACHED_LOCATION],
binary=True)
lock_type = "MEMCACHED"
except Exception:
from django.core.cache import cache
from contextlib import contextmanager
lock_type = "MEMCACHED-LOCAL-CONTEXT"
memcache_client = None
It would be probably better to change this and rename the variable so that it is very clear which one enables what, and also having the MEMCACHED_ENABLED
driving the tasks
behavior.
We need to revise completely this one I'm afraid.
@afabiani I am unsure what you are referring to by this?
@gannebamm what do you want to say by this? Should the memcached configuration not be visible to "end-users"? Is an admin operating GeoNode an end-user here? |
@EHJ-52n please take a look here as you can see the locking mechanism is created only taking into account two conditions:
import pylibmc
import sherlock
from sherlock import MCLock as Lock
sherlock.configure(
expire=settings.MEMCACHED_LOCK_EXPIRE,
timeout=settings.MEMCACHED_LOCK_TIMEOUT)
memcache_client = pylibmc.Client(
[settings.MEMCACHED_LOCATION],
binary=True)
lock_type = "MEMCACHED" no check is done against the |
I was referring to the checklist item
And was thinking out loud, that we do not need an issue for that PR, since it is not something end-users will face. |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @EHJ-52n on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added. |
Are you recommending to rephrase MEMCACHED_LOCK_EXPIRE to SHERLOCK_LOCK_EXPIRE or CACHE_LOCK_EXPIRE and MEMCACHED_LOCK_TIMEOUT, too? In addition, surrounding the memcache_client initialization with a check for MEMCACHE_ENABLED? try:
import pylibmc
import sherlock
from sherlock import MCLock as Lock
sherlock.configure(
expire=settings.SHERLOCK_LOCK_EXPIRE,
timeout=settings.SHERLOCK_LOCK_TIMEOUT)
if settings.MEMCACHED_ENABLED:
memcache_client = pylibmc.Client(
[settings.MEMCACHED_LOCATION],
binary=True)
lock_type = "MEMCACHED"
else:
raise Exception
except Exception:
from django.core.cache import cache
from contextlib import contextmanager
lock_type = "MEMCACHED-LOCAL-CONTEXT"
memcache_client = None I am not sure if the else case is required because I did not fully check the task module. |
@EHJ-52n can you please take a look at the conflicts? |
Goal: one service per container - add own service for memcached - remove memcached from geonode image - add MEMCACHED_OPTIONS variable to env files - enable memcached by default
dbc8702
to
2ed1236
Compare
@giohappy Fixed the conflicts. Are you okay with the changes? |
@EHJ-52n I have gone through the PR and comments. IMHO the thing about Possible solutions (for another task):
|
@EHJ-52n please align geonode-project once this is merged |
@giohappy working on this - see GeoNode/geonode-project#479. |
Description
Goal: "one service per container"
Background
Changes
enable memcached by defaultChecklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.