divijvaidya commented on a change in pull request #1556:
URL: https://github.com/apache/tinkerpop/pull/1556#discussion_r800879591
##########
File path: gremlin-go/go.mod
##########
@@ -0,0 +1,18 @@
+module github.com/lyndonb-bq/tinkerpop/gremlin-go
+
+go 1.17
+
+require (
+ github.com/google/uuid v1.3.0
Review comment:
please ensure that the libraries satisfies the Apache requirement for
third party libraries. https://www.apache.org/legal/resolved.html#category-a
##########
File path: gremlin-go/driver/connection.go
##########
@@ -0,0 +1,68 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+type connection struct {
Review comment:
From my experience of maintaining other clients, I have observed that
making a concept of `state` in multiple client entities could be very useful
for debugging.
My suggestion would be to add stateful transition to the lifecycle of a
connection such as:
Initialised
Connection With Remote Initiated
Connection With Remote Established
Close request received
Close in progress
Closed
These state transitions help us model various edge cases such as what
happens when a connection receives a message to disconnect when it is in
"Connection With Remote Initiated" state.
##########
File path: gremlin-go/driver/connection.go
##########
@@ -0,0 +1,68 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+type connection struct {
+ host string
+ port int
+ transporterType TransporterType
+ logHandler *logHandler
+ transporter transporter
+ protocol protocol
+ results map[string]ResultSet
+}
+
+func (connection *connection) close() (err error) {
+ if connection.transporter != nil {
+ err = connection.transporter.Close()
+ }
+ return
+}
+
+func (connection *connection) connect() error {
+ if connection.transporter != nil {
+ closeErr := connection.transporter.Close()
+ connection.logHandler.logf(Warning, transportCloseFailed,
closeErr)
+ }
+ connection.protocol = newGremlinServerWSProtocol(connection.logHandler)
+ connection.transporter = getTransportLayer(connection.transporterType,
connection.host, connection.port)
+ err := connection.transporter.Connect()
+ if err != nil {
+ return err
+ }
+ connection.protocol.connectionMade(connection.transporter)
+ return nil
+}
+
+func (connection *connection) write(request *request) (ResultSet, error) {
+ if connection.transporter == nil || connection.transporter.IsClosed() {
+ err := connection.connect()
Review comment:
Let's keep things simple and not retry to re-connect here. A connection
should be in "connected" state if we want to write on it. Else throw an error
about illegal state and the upper layers should choose a different connection
to work with.
##########
File path: gremlin-go/driver/serializer.go
##########
@@ -0,0 +1,343 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+import (
+ "bytes"
+ "encoding/binary"
+ "errors"
+ "math/big"
+ "strings"
+
+ "github.com/google/uuid"
+)
+
+const graphBinaryMimeType = "application/vnd.graphbinary-v1.0"
Review comment:
note that graphbinary has one more mime type:
`application/vnd.graphbinary-v1.0-stringd`
##########
File path: gremlin-go/driver/protocol.go
##########
@@ -0,0 +1,127 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+import (
+ "errors"
+ "fmt"
+ "net/http"
+)
+
+type protocol interface {
Review comment:
please document the responsibility of this class. More specifically,
please explain somewhere the contract between the transporter, connection and
protocol. Please also explain how their lifecycles impact each other.
##########
File path: gremlin-go/driver/protocol.go
##########
@@ -0,0 +1,127 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+import (
+ "errors"
+ "fmt"
+ "net/http"
+)
+
+type protocol interface {
+ connectionMade(transport transporter)
+ read(resultSets map[string]ResultSet) (string, error)
+ write(request *request, results map[string]ResultSet) (string, error)
+}
+
+type protocolBase struct {
+ protocol
+
+ transporter transporter
+}
+
+type gremlinServerWSProtocol struct {
+ *protocolBase
+
+ serializer serializer
+ logHandler *logHandler
+ maxContentLength int
+ username string
+ password string
+}
+
+func (protocol *protocolBase) connectionMade(transporter transporter) {
+ protocol.transporter = transporter
+}
+
+func (protocol *gremlinServerWSProtocol) read(resultSets map[string]ResultSet)
(string, error) {
+ // Read data from transport layer.
+ msg, err := protocol.transporter.Read()
+ if err != nil || msg == nil {
+ if err != nil {
+ return "", err
+ }
+ protocol.logHandler.log(Error, malformedURL)
+ return "", errors.New("malformed ws or wss URL")
+ }
+ // Deserialize message and unpack.
+ response, err := protocol.serializer.deserializeMessage(msg)
+ if err != nil {
+ return "", err
+ }
+
+ responseID, statusCode, metadata, data := response.responseID,
response.responseStatus.code,
+ response.responseResult.meta, response.responseResult.data
+
+ resultSet := resultSets[responseID.String()]
+ if resultSet == nil {
+ resultSet = newChannelResultSet(responseID.String())
+ }
+ resultSets[responseID.String()] = resultSet
+ if aggregateTo, ok := metadata["aggregateTo"]; ok {
+ resultSet.setAggregateTo(aggregateTo.(string))
+ }
+
+ // Handle status codes appropriately. If status code is
http.StatusPartialContent, we need to re-read data.
+ if statusCode == http.StatusProxyAuthRequired {
Review comment:
please move handling of status code in a separate function.
##########
File path: gremlin-go/driver/graphTraversalSource.go
##########
@@ -0,0 +1,23 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package gremlingo
+
+type GraphTraversalSource interface {
Review comment:
this and other data model objects associated with Graph should perhaps
be separated from connection related stuff as driver/structure/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
