Skip to content

Add redis backend support using redis streams - #14

Merged
marcin-bakowski-dev merged 12 commits into
cloudchacho:mainfrom
marcin-bakowski-dev:redis_backend_wip
Feb 27, 2025
Merged

Add redis backend support using redis streams#14
marcin-bakowski-dev merged 12 commits into
cloudchacho:mainfrom
marcin-bakowski-dev:redis_backend_wip

Conversation

@marcin-bakowski-dev

@marcin-bakowski-devmarcin-bakowski-dev commented Feb 20, 2025

Copy link
Copy Markdown
Collaborator

Add redis backend support using redis streams.

I extended the original pr: #11 and applied few minor changes:

  • add block=500 param to xreadgroup() method to block read operation for 500ms if there is no messages available in the stream to avoid making too many I/O operations in the consumer read loop. Without blocking, the network transfer between consumer and redis server is about 2-3Mbps all the time. Consumer instance cpu usage is also increased.
  • redis operations, which move messages from/to dlq, are group in the pipeline to support transactions
  • add docker redis server instance to use it during redis backend unit tests. I tried to use fakeredis (https://pypi.org/project/fakeredis/), but it's not fully compatible with actual redis responses (ex: xreadgroup() - redis returns dict, fakeredis returns list of tuples, xpending() result doesn't include times_delivered in fakeredis implementation).

@marcin-bakowski-dev
marcin-bakowski-devforce-pushed the redis_backend_wip branch 3 times, most recently from 4d662ab to 8091abdCompareFebruary 20, 2025 15:49
@marcin-bakowski-dev
marcin-bakowski-devforce-pushed the redis_backend_wip branch 5 times, most recently from 55efa05 to 87406d6CompareFebruary 20, 2025 17:04
@codecov

codecovBot commented Feb 20, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 98.23529% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.13%. Comparing base (4666912) to head (defe3ae).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
hedwig/backends/redis.py97.90%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #14 +/- ##
==========================================
+ Coverage 96.48% 97.13% +0.65% 
==========================================
Files 27 28 +1 Lines 1508 1643 +135 ==========================================
+ Hits 1455 1596 +141 + Misses 53 47 -6 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tags: hedwig-python-${{ matrix.python_version }}:latest

- name: Build docker compose image
run: make build

@marcin-bakowski-devmarcin-bakowski-devFeb 21, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added make build (docker compose build ) as a separate step, because docker compose run doesn't allow to provide build args and it always builds docker image for default SC_PYTHON_VERSION=3.13. During make build docker SC_PYTHON_VERSION arg value is taken from SC_PYTHON_VERSION env var. Docker build image operation takes only few seconds since it uses all the layers from the cache, which is prepared in previous Build and push Docker image step.

Comment threadMakefile Outdated

release: release_setup
@docker compose run --rm app ./scripts/release.sh
@docker compose run --rm \

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should run release script outside docker container to avoid passing sensitive data via env vars.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not a bad idea. It's a specialised process only run on a few people's laptops.

def post_process_hook_kwargs(queue_message) -> dict:
return {}

@abc.abstractmethod

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming we want to require each subclass to implement all abstract methods, the base classes needs to inherit from abc.ABC or have abc.ABCMeta as a meta class for the abstractmethod decorator to raise an error on instantiation of subclasses that don't implement the method(s).

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, thanks

@holmandwholmandw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! @maroux do you want to take a final look on where it stands?

@marouxmaroux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!

@marcin-bakowski-dev
marcin-bakowski-dev merged commit 38b19d7 into cloudchacho:mainFeb 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@marcin-bakowski-dev@maroux@holmandw