-
Notifications
You must be signed in to change notification settings - Fork 390
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
Protocol v1.0 - part 1 #3814
Protocol v1.0 - part 1 #3814
Conversation
975369e
to
df6f627
Compare
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.
Dropping pre 0.13 looks OK.
As for copying the protocol - this is fine. I going to implement a different approach:
binary.pyx: EdgeConnection
binary_v0.pyx: EdgeConnectionBackwardsCompatible extends EdgeConnection
binary_v0.pyx
would have v0 methods prefixed with def legacy_
.
Then protocol.pyx
could just always instantiate EdgeConnectionBackwardsCompatible
Now, I think your approach also works so let's go with it.
Thanks for the quick response! That's exactly what I wanted to know. However with my current approach, there're some complication of like switching the protocol object attached in the transport, and some in-and-out in the server. I'll try the inheritance approach very quickly and decide from there. |
9b0339f
to
9997a4e
Compare
Also rename make_describe_msg to make_command_data_description_msg
The OptimisticExecute was not ready for the Parse/Execute convention because it was always looking up in the cache and ignoring the Parse result. This was an issue because some statement is not cacheable and compiling again confused the state machine (#3120). Fixed in a way that the previously parsed result is used if the query hash matches, and dropped immediately on _execute() to mimic the legacy OptimisticExecute behavior. test_server_proto_tx_03() and _04() covers both cases.
The reason it was kept is explained in #3120, mitigated in #3814 #3837. But the core issue that compiler assumes mutation of transaction state for any compilation even by Parse message still exists. We can drop the `_last_anon_compiled` because the client bindings are no longer sending Parse + Execute for `commit` or `rollback` commands (which triggered two server compilation of the same command) like edgedb/edgedb-python#337. However, we still need to double check all bindings to make sure the transaction control commands are using only one `Execute` message.
Pre-RFC: #3772
Depends on edgedb/edgedb-python#306