Skip to content
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

Offload replication writes to IO threads #1485

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

uriyage
Copy link
Contributor

@uriyage uriyage commented Dec 24, 2024

This PR offloads the write to replica clients to IO threads.

Main Changes

  • Replica writes will be offloaded but only after the replica is in online mode..
  • Replica reads will still be done in the main thread to reduce complexity and because read traffic from replicas is negligible.

Implementation Details

In order to offload the writes, writeToReplica has been split into 2 parts:

  1. The write itself made by the IO thread or by the main thread
  2. The post write where we update the replication buffers refcount will be done in the main-thread after the write-job is done in the IO thread (similar to what we do with a regular client)

Additional Changes

  • In writeToReplica we now use writev in case more than 1 buffer exists.
  • Changed client nwritten field to ssize_t since with a replica the nwritten can theoretically exceed int size (not subject to NET_MAX_WRITES_PER_EVENT limit).
  • Changed parsing code to use memchr instead of strchr:
    • During parsing command, ASAN got stuck for unknown reason when called to strchr to look for the next \r
    • Adding assert for null-terminated querybuf didn't resolve the issue.
    • Switched to memchr as it's more secure and resolves the issue

Testing

  • Added integration tests
  • Added unit tests

Related issue: #761

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@uriyage uriyage force-pushed the replication-offload branch from 3aee49b to 846c816 Compare December 24, 2024 07:24
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 76.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 70.85%. Comparing base (980a801) to head (846c816).
Report is 19 commits behind head on unstable.

Files with missing lines Patch % Lines
src/io_threads.c 0.00% 14 Missing ⚠️
src/networking.c 88.23% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1485      +/-   ##
============================================
+ Coverage     70.78%   70.85%   +0.07%     
============================================
  Files           119      119              
  Lines         64691    64928     +237     
============================================
+ Hits          45790    46005     +215     
- Misses        18901    18923      +22     
Files with missing lines Coverage Δ
src/replication.c 87.54% <100.00%> (+0.03%) ⬆️
src/server.h 100.00% <ø> (ø)
src/networking.c 88.39% <88.23%> (+0.09%) ⬆️
src/io_threads.c 6.82% <0.00%> (-0.70%) ⬇️

... and 25 files with indirect coverage changes

Sign up for free to 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.

1 participant