Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Design documentation for adding a raw-FFI thread manager #31
base: main
Are you sure you want to change the base?
Design documentation for adding a raw-FFI thread manager #31
Changes from 6 commits
38791cf
29e2244
32b751e
799b248
a86e424
265daa0
8d43095
11ea2c7
d7325a1
fe1690a
744e92f
56ebe90
64e034a
2d688c1
f6b702b
af4e2a4
df00c54
ce3eb6c
28c672b
91490bc
7058b02
f339390
c4a13da
8bb74ba
eece5f9
73ba55b
b88cbbd
30406bf
041bd39
a25467e
0062ed9
ae090e6
a852c04
50bc303
fdd659a
c3b7ae6
d5edd9d
3bd84ba
26bafc1
765d220
969a896
3896446
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Redis commands can be presented as a simple string array, so passing protobuf messages from the wrapper to the core adds unnecessary complication when we're talking about a raw FFI solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. we'll have a generic execute_command function in rust that excepts a string array and all FFI functions from the wrapper will call it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please take few examples of complex commands with many arguments, like sorted set or list for example https://lettuce.io/core/release/api/io/lettuce/core/api/sync/RedisSortedSetCommands.html.
Lets try to better undersrtand it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asafpamzn also in the UDS solution we convert all passed arguments into a string array before passing it to the core. so there's no issue when passing complex commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to verify that all RESP3 value types can be returned using C data types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is addressed now, since we will starting with RESP2 for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference where these errors came from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename file - it shows both raw FFI and UDS approaches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename all references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not correct. May be we need to split diagram into 2 or 3 ones: java to UDS, UDS to rust and java-uds-rust in zoom out mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move go to another doc too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather hold off on splitting until we have an idea of where these documents will be stored.