-
Notifications
You must be signed in to change notification settings - Fork 844
TINKERPOP-3219 Expose serialization functions for alternative transport protocols in gremlin-go #3285
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
base: 3.8-dev
Are you sure you want to change the base?
Conversation
1606cd1 to
d71e06d
Compare
|
You mentioned that:
Could you elaborate on that? I'd be curious to know which ones are offering this that support TinkerPop. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.8-dev #3285 +/- ##
==========================================
Coverage ? 76.47%
Complexity ? 14855
==========================================
Files ? 1159
Lines ? 71974
Branches ? 8031
==========================================
Hits ? 55039
Misses ? 14006
Partials ? 2929 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
That's an overstating description. gRPC isn't widespread yet. I'm working for eBay and we have an internal graph DB that is using gRPC for connection. |
|
It seems that the other driver GLVs already expose the serialization layer so go is an outlier. Perhaps instead of using an exposed wrapper we can just make the serializer public for consistency with the other GLVs. |
|
+1 to Andrea's question above. Unless there are other justifications I'm not aware of, I would be more in favour of updating |
d71e06d to
7fe1815
Compare
|
Thanks for the feedback! The PR is updated:
|
0ccfa5d to
e619252
Compare
e619252 to
e672a23
Compare
thanks, that's interesting. i've heard tell of the eBay graph. thanks for contributing. i'd actually like to hear more about this if you're willing to share. For instance, do you plug-in your own |
|
Hi @spmallette !
|
| // } | ||
| // resultSet := NewResultSet("request-123", results) | ||
| // allResults, _ := resultSet.All() | ||
| func NewResultSet(requestID string, results []*Result) ResultSet { |
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 am curious why you opted to create a new constuctor instead of changing an existing one to be public? Was there something lacking from the existing ones which made them not usable for your use cases?
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.
The existing constructor signature is:
func newChannelResultSet(requestID string, container *synchronizedMap) ResultSet
The synchronizedMap parameter comes from connection.results and mainly serve for connection-based, concurrent request handling, which is not needed in my request-based use case.
Using newChannelResultSet would probably mean to make public addResult, synchronizedMap along with itself, and even something from connection.go, making everything complicated.
This new constructor enables Submitxxx to return a ResultSet from Response.ResponseResult.Data, which aligns with current interfaces
|
Hi @DR1N0, Thanks for making all of the requested changes. All of the updates look good to me. VOTE +1 |
Motivation
The current gremlin-go driver only supports WebSocket transport. However, many modern graph database deployments use alternative protocols like gRPC for better performance, streaming capabilities, and infrastructure compatibility.
The serialization logic (GraphBinary) in gremlin-go is currently internal/private, preventing developers from building custom transport implementations while maintaining Gremlin API compatibility.
JIRA: TINKERPOP-3219
Changes
This PR exposes existing internal serialization functions by making them public, bringing gremlin-go in line with other GLVs (Python, .NET, JavaScript) that already expose their serialization layers.
Functions Made Public
In
request.go:MakeBytecodeRequest()- Creates a request from bytecode (formerlymakeBytecodeRequest)MakeStringRequest()- Creates a request from string query (formerlymakeStringRequest)In
serializer.go:SerializeMessage()- Serializes request to GraphBinary format (formerlyserializeMessage)DeserializeMessage()- Deserializes GraphBinary to response (formerlydeserializeMessage)Serializerinterface for completenessTypes Made Public
To enable usage of the serialization functions, the following types and their fields were made public:
Request handling:
GraphBinarySerializer- The serializer implementationResponse handling:
Response- Response structure with exported fieldsResponseStatus- Status information (code, message, attributes)ResponseResult- Result data and metadataThis ensures users can create requests, serialize them, deserialize responses, and access the result data.
New Functions
NewResultSet(requestID, results)- Creates ResultSet from collected resultsUse Case
Enables building custom transport implementations for Gremlin queries:
This is particularly valuable for:
Example Usage
Notes
This approach was chosen based on reviewer feedback suggesting consistency with other GLVs rather than adding a wrapper layer. The direct exposure of serialization functions is simpler and more maintainable.