Fix issues caused by differences between redis and elasticache (PP-1693) #2045
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2045 +/- ##
==========================================
+ Coverage 90.77% 90.78% +0.01%
==========================================
Files 342 343 +1
Lines 40513 40563 +50
Branches 8770 8788 +18
==========================================
+ Hits 36774 36824 +50
Misses 2484 2484
Partials 1255 1255 ☔ View full report in Codecov by Sentry. |
| ) | ||
| ) |
There was a problem hiding this comment.
The changes in src/palace/manager/celery/tasks/marc.py could come in as a separate PR if desired. They fix a concurrency issue I found while doing other testing. Occasionally, since we were re-queuing while holding the lock, another worker would pick up the new task before this one had released the lock. So this updates the order of operations, so we release the lock before calling task.replace.
| This function validates that all the results of the pipeline are successful, | ||
| and not a ResponseError. | ||
|
|
||
| NOTE: The AWS ElastiCache implementation returns slightly different results then Redis. |
There was a problem hiding this comment.
Minor typo: "... [than] Redis."
| else: | ||
| if char == self._ESCAPE_CHAR: | ||
| in_escape = True | ||
| else: | ||
| unescaped.append(char) |
There was a problem hiding this comment.
Minor: If you aren't just aiming for a parallel structure for testing char, this could probably be de-indented to:
if in_escape:
...
elif char == self._ESCAPE_CHAR:
in_escape = True
else:
unescaped.append(char)| self.mock_upload_key_3 = "test3" | ||
| # Some keys with special characters to make sure they are handled correctly. | ||
| self.mock_upload_key_1 = "test/test1/?$xyz.abc" | ||
| self.mock_upload_key_2 = "t'est💣/tëst2.\"ext`" |
Description
Resolves issues surfaced on Minotaur when running against ElastiCache instead of Redis. These issues were around the escaping of characters in object key names. This seems to be handled differently in Redis vs Elasticache. In order to work around this, I ended up creating a scheme where we escaping the problematic characters.
I don't like this code, or that we are depending on behaviour that is hard to test on CI, but it does fix the issue. If this continues to be an issue, we may either have to rewrite the MARC redis model, so that it doesn't depend on JSON or try to figure out how we can test directly with ElastiCache in CI.
Motivation and Context
Fix issues that surfaced after deployment to Minotaur.
How Has This Been Tested?
Checklist