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

Prism Query Interface: Replace gRPC with custom handling of messages #495

Merged
merged 2,407 commits into from
May 16, 2024

Conversation

gartens
Copy link
Contributor

@gartens gartens commented Apr 19, 2024

This removes gRPC in favor of sending/receiving the protobuf encoded messages ourselves. This gives us better control to rollback transactions immediately when the connection is closed, allows us to support different transport methods (TCP and UNIX) and depending on the transport to authenticate clients directly (UNIX).

Various messages and serializations are improved as well.

Additional Changes

  • Ways to distinguish between auto generated and manual queries
  • Constratint enforcement, moved the physical execution to the commit, to prevent physical rollback challenges

Fixes

  • Restore of IdBuilder
  • Large number on various stores
  • Correct Result number in Neo4j for prepared queries
  • Added missing commits for some Neo4j DD queries

ToDo

  • Delete socket files when stopping polypheny?
  • Fix neo4J test

gartens and others added 30 commits April 9, 2024 13:28
This may return in the future as a non-unique "client descriptor" to help admins determine who is using the interface.  But for now it is no longer needed to identify client sessions (this is now done with the socket).
This fixes a rather intricate bug: The ResultIterator would use the batch size passed on creation, but the fetch code would it compare with the fetch size specified in the message.  This would lead to fetch reporting isLast to early, when the fetch size in the message is different from in the initial execute message.
This way seems more logic.  Unparameterized statements are still a bit of a hack, but with the async changes, it should be possible to not send the statement id, but being able to cancel calls based on the message id.

While working on the loop, change to allow the ConnectionRequest only once and only as the first message.
Copy link
Member

@datomo datomo left a comment

Choose a reason for hiding this comment

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

Thx @gartens for this PR, this looks good to me.

@datomo
Copy link
Member

datomo commented May 16, 2024

@gartens is this ready for review and can be changed from draft, or do you have anything to add?

@gartens
Copy link
Contributor Author

gartens commented May 16, 2024

Go ahead @datomo.

Copy link
Member

@vogti vogti left a comment

Choose a reason for hiding this comment

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

Thx, @gartens, @TobiasHafner, and @datomo, for this PR!

@vogti vogti marked this pull request as ready for review May 16, 2024 13:47
@vogti vogti merged commit d16fe55 into master May 16, 2024
34 of 35 checks passed
@vogti vogti deleted the proto-without-grpc branch May 16, 2024 13:56
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.

3 participants