Skip to content

feat(server): socket migration across shard - #2476

Merged
spetz merged 31 commits into
apache:masterfrom
tungtose:socket-transfer
Dec 21, 2025
Merged

feat(server): socket migration across shard#2476
spetz merged 31 commits into
apache:masterfrom
tungtose:socket-transfer

Conversation

@tungtose

@tungtose tungtose commented Dec 12, 2025

Copy link
Copy Markdown
Contributor

This PR addressing #2386

Enable TCP socket migration across shards. When a client sends messages to the wrong shard, migrate the socket to the correct shard to eliminate cross-shard forwarding overhead

@tungtose tungtose changed the title feat(performance): socket migration across shard feat(server): socket migration across shard Dec 12, 2025
hubcio
hubcio reviewed Dec 12, 2025
View reviewed changes
Comment thread Cargo.toml Outdated
@tungtose
constructed socket transfer msg
7b7b6ac 
add handler result to control lifecycle

add the point failed to login

at the point the connection hanging, wait for the new recover tcp stream

convert handle_request & handle_event take &Rc<IggyShard> in order to spawn a connection inside the message handler

stucking with the single client polling tests

need to debug test background_graceful_shutdown

all tests pass now

self review
@tungtose
self review
b72477b
@tungtose
add feature config flag
cf47bb3
@tungtose
noop
e85bb7d
@tungtose
fix sort deps
ca21259
@tungtose
attempt to move to nix
e9dcc95
@tungtose
use nix wrapper instead libc
dec24e1
@tungtose
format toml
b1c4c16
@tungtose
noop
9f881ec
@tungtose
commit lock file
198a24a
hubcio
hubcio reviewed Dec 15, 2025
View reviewed changes

@hubcio hubcio 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.

overall, design in this PR is sound, however i'd like you to consider: all 50+ handlers now return HandlerResult but only SendMessages use Migrated. have you thought about adding IggyError::ConnectionMigrated(to_shard) and catch it in connection handler? this would greatly reduce changed files in the PR

@spetz @numinnex opinions?

Comment thread core/server/src/shard/handlers.rs Outdated
Comment thread core/server/src/shard/handlers.rs Outdated
Comment thread core/server/src/binary/handlers/messages/send_messages_handler.rs Outdated
Comment thread core/server/src/binary/handlers/messages/send_messages_handler.rs Outdated
Comment thread core/server/src/streaming/session.rs
Comment thread core/configs/server.toml
@tungtose

tungtose commented Dec 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

overall, design in this PR is sound, however i'd like you to consider: all 50+ handlers now return HandlerResult but only SendMessages use Migrated. have you thought about adding IggyError::ConnectionMigrated(to_shard) and catch it in connection handler? this would greatly reduce changed files in the PR

@spetz @numinnex opinions?

I actually did that in the first place, but on second thought, this is not an error, so I have to change all of the handler results. I would be happy to change it if you prefer to treat it as an error

@hubcio

hubcio commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

overall, design in this PR is sound, however i'd like you to consider: all 50+ handlers now return HandlerResult but only SendMessages use Migrated. have you thought about adding IggyError::ConnectionMigrated(to_shard) and catch it in connection handler? this would greatly reduce changed files in the PR
@spetz @numinnex opinions?

I actually did that in the first place, but on second thought, this is not an error, so I have to change all of the handler results. I would be happy to change it if you prefer to treat it as an error

I mean your approach does make sense... Lets wait for other core devs opinion.

@tungtose

tungtose commented Dec 16, 2025

Copy link
Copy Markdown
Contributor Author

@hubcio I have resolved all comments and fixed the C# tests

hubcio
hubcio reviewed Dec 17, 2025
View reviewed changes
Comment thread core/server/src/binary/handlers/messages/send_messages_handler.rs Outdated
Comment thread core/common/src/sender/mod.rs
Comment thread core/server/src/streaming/utils/crypto.rs Outdated
Comment thread core/common/src/sender/mod.rs Outdated
hubert-laserdata
hubert-laserdata reviewed Dec 19, 2025
View reviewed changes
Comment thread core/server/src/binary/handlers/messages/send_messages_handler.rs
hubcio
hubcio approved these changes Dec 19, 2025
View reviewed changes

@hubcio hubcio 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.

LGTM.

@numinnex can you check this too?

@spetz
spetz merged commit 96c3b1f into apache:master Dec 21, 2025
53 checks passed
@tungtose tungtose mentioned this pull request Dec 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spetz spetz spetz approved these changes
@hubcio hubcio hubcio approved these changes
@numinnex numinnex numinnex approved these changes
+1 more reviewer
@hubert-laserdata hubert-laserdata hubert-laserdata left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tungtose @hubcio @spetz @numinnex @hubert-laserdata