Updated memcache error checking to handle large object exception#748
Updated memcache error checking to handle large object exception#748
Conversation
googlebot
added
the
cla: yes
| if not memcache.add('{}:greetings'.format(guestbook_name), | ||
| greetings, 10): | ||
| logging.error('Memcache set failed.') | ||
| except: |
There was a problem hiding this comment.
Catch a more specific exception
| try: | ||
| if not memcache.add('{}:greetings'.format(guestbook_name), | ||
| greetings, 10): | ||
| logging.error('Memcache set failed.') |
There was a problem hiding this comment.
since these are two separate cases, you want two separate error messages. Should be, Memcache add failed, key already exists and Memcache add failed, data larger than 1MB
There was a problem hiding this comment.
Added a more specific message for data larger than 1MB.
For the memcache.add False result, it might not necessarily be a key already exists error, so I'm leaving it as 'Memcache set failed.'
| if not memcache.add('{}:greetings'.format(guestbook_name), | ||
| greetings, 10): | ||
| try: | ||
| if not memcache.add('{}:greetings'.format(guestbook_name), |
There was a problem hiding this comment.
Maybe break up this cramped up line with added = memcache.add() then if not added
| added = memcache.add('{}:greetings'.format(guestbook_name), | ||
| greetings, 10) | ||
| if not added: | ||
| logging.error('Memcache set failed.') |
There was a problem hiding this comment.
just need -value already exists here right?
There was a problem hiding this comment.
Do we know for sure if memcache.add returning False is necessarily a 'key already exists' type of error? The docs say memcache.add returns 'False on error'. (https://cloud.google.com/appengine/docs/python/refdocs/modules/google/appengine/api/memcache#Client.add). Wondering if the False could mean something else in certain circumstances.
There was a problem hiding this comment.
Yeah I don't know, I guess we can leave it as is.
| greetings, 10): | ||
| logging.error('Memcache set failed.') | ||
| try: | ||
| added = memcache.add('{}:greetings'.format(guestbook_name), |
There was a problem hiding this comment.
please avoid hanging indents, newline after add(.
| logging.error('Memcache set failed.') | ||
| except ValueError: | ||
| logging.error('Memcache set failed - data larger than 1MB') | ||
| except Exception as e: |
There was a problem hiding this comment.
No need for this broad exception.
* feat: add dual region bucket support and tests * add dual region bucket sample * fix lint * update docstrings and doc ref links Co-authored-by: Daniel Bankhead <dan@danielbankhead.com>
4 participants
See bug #33780149 on buganizer.
For some reason memcache.add throws an exception for objects larger than 1 MB rather than simpling returning False. Thus, I changed the sample code to use try/except around memcache.add, while preserving the check to see if memcache.add returned True or False. The boolean returned does seem to be necessary since it is False when memcache already has an entry for a given key.
Also got rid of 'version: 1' from the app.yaml so that 'gcloud app deploy' works.