You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/04/13 01:19:37 UTC
[GitHub] [tinkerpop] lyndonbauto opened a new pull request, #1619: Gremlin Go MS5 - Feature Complete
lyndonbauto opened a new pull request, #1619:
URL: https://github.com/apache/tinkerpop/pull/1619
This has the changes for Milestone 5 of the gremlin-go driver. I would like to thank @xiazcy, @simonz-bq, @L0Lmaker, @vkagamlyk, @spmallette, and @krlawrence for continued help they have all been putting into this.
We have all tests passing in the Cucumber test suite, have integrated the driver into the TinkerPop build infrastructure and have added all remaining features to the driver at this point.
From here we will be working on enhancing documentation, adding performance testing, and bug fixes to any issues which are exposed.
Pull request comments will be addressed as part of the next milestone.
This pull request will remain open for at least a week, so the community has a chance to look at it and critique it.
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#discussion_r851224088
##########
gremlin-go/Dockerfile:
##########
@@ -0,0 +1,39 @@
+# 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.
+
+ARG GREMLIN_SERVER_VERSION=${version:-3.5.4}
+FROM tinkerpop/gremlin-server:${GREMLIN_SERVER_VERSION}-SNAPSHOT
Review Comment:
i'm not sure i understand how this works as docker is still something i'm learning. i understand what an `ARG` is but i don't follow what the implications are of hardcoding to 3.5.4 and then appending "-SNAPSHOT". what happens when we are doing build artifacts on 3.5.4 and we are no longer on "-SNAPSHOT"? Isn't there a nice way to gather the version from the `pom.xml` and pass it through so that all of this stays dynamic?
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#discussion_r851224860
##########
gremlin-go/Dockerfile:
##########
@@ -0,0 +1,39 @@
+# 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.
+
+ARG GREMLIN_SERVER_VERSION=${version:-3.5.4}
+FROM tinkerpop/gremlin-server:${GREMLIN_SERVER_VERSION}-SNAPSHOT
+
+USER root
+RUN mkdir -p /opt
+WORKDIR /opt
+COPY gremlin-server/src/test /opt/test/
+COPY gremlin-go/docker/docker-entrypoint.sh gremlin-go/docker/*.yaml /opt/
+RUN chmod 755 /opt/docker-entrypoint.sh
+
+# Setting to 3.5.3 does gives error even when using outside of Docker.
+ENV NEO4J_VERSION=${version:-3.5.2}
Review Comment:
Unless I'm misunderstanding this section, pinning to a version is problematic especially to an older version. The version should come from the `pom.xml` so that it stays current. Is it possible there is an error because `neo4j-gremlin` needs to be explicitly built for docker's purposes?
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#discussion_r851216687
##########
gremlin-console/bin/gremlin.sh:
##########
@@ -1 +1,21 @@
+#!/bin/bash
+#
+#
+# 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.
+#
Review Comment:
dont think this needs a license - it's just a symlink. wondering if it actually breaks the symlink to add that? in any case, if it's being recognized as text in a windows build and failing rat for some reason, maybe just add an exclusion for it.
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto commented on pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#issuecomment-1104385594
@divijvaidya Thank you for looking over the code! I am going to be addressing these comments as part of the MS6 delivery as discussed. The feedback is really appreciated :)
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#discussion_r853255058
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
Review Comment:
What happens when we call close() while a `createConnection()` is in progress? I think we would close the registered connections and would never close the connection that will be added to the pool after `createConnection()` is complete.
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
+ // Close and purge connections from pool if there is more than one being unused
+ if leastUsed != nil && (leastUsed.activeResults() == 0 && connection.activeResults() == 0) {
+ // Close the connection asynchronously since it is a high-latency method
+ go func() {
+ pool.logHandler.log(Debug, closeUnusedPoolConnection)
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }()
+
+ continue
+ }
+
+ // Mark connection as valid to keep
+ pool.connections[validIndex] = connection
+ validIndex++
+
+ // Set the least used connection
+ if leastUsed == nil || connection.activeResults() < leastUsed.activeResults() {
+ leastUsed = connection
+ }
+ } else {
+ pool.logHandler.log(Debug, purgingDeadConnection)
+ }
+ }
+
+ // Deallocate truncated dead connections to prevent memory leak
+ for invalidIndex := validIndex; invalidIndex < len(pool.connections); invalidIndex++ {
+ pool.connections[invalidIndex] = nil
+ }
+ pool.connections = pool.connections[:validIndex]
+
+ // Create new connection if no valid connections were found in the pool or the least used connection exceeded
+ // the concurrent usage threshold while the pool still has capacity for a new connection
+ if leastUsed == nil ||
+ (leastUsed.activeResults() >= pool.newConnectionThreshold && len(pool.connections) < cap(pool.connections)) {
+ return pool.newConnection()
+ } else {
+ return leastUsed, nil
+ }
+ }
+}
+
+func (pool *loadBalancingPool) newConnection() (*connection, error) {
+ connection, err := createConnection(pool.url, pool.logHandler, pool.connSettings)
+ if err != nil {
+ return nil, err
+ }
+ pool.connections = append(pool.connections, connection)
+ return connection, nil
+}
+
+func newLoadBalancingPool(url string, logHandler *logHandler, connSettings *connectionSettings, newConnectionThreshold int,
+ maximumConcurrentConnections int) (connectionPool, error) {
+ pool := make([]*connection, 0, maximumConcurrentConnections)
+ initialConnection, err := createConnection(url, logHandler, connSettings)
Review Comment:
The number of initial connections per pool should be configurable. We have faced situations with Java client where users complained about the fact that "lazy initialization" leads to higher latency for first few requests to the server.
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
+ // Close and purge connections from pool if there is more than one being unused
+ if leastUsed != nil && (leastUsed.activeResults() == 0 && connection.activeResults() == 0) {
+ // Close the connection asynchronously since it is a high-latency method
+ go func() {
+ pool.logHandler.log(Debug, closeUnusedPoolConnection)
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }()
+
+ continue
+ }
+
+ // Mark connection as valid to keep
+ pool.connections[validIndex] = connection
+ validIndex++
+
+ // Set the least used connection
+ if leastUsed == nil || connection.activeResults() < leastUsed.activeResults() {
+ leastUsed = connection
+ }
+ } else {
+ pool.logHandler.log(Debug, purgingDeadConnection)
Review Comment:
should there be a function call here to purge connections?
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
+ // Close and purge connections from pool if there is more than one being unused
Review Comment:
I am not very sure about this logic. Connection setup is quite expensive due to SSL handshake, authn and authz happening with every setup. It is very likely that a connection may be intermittently un-used and this logic will end up creating and destroying connections all the time!
Alternatively, we can do the following:
1. Close "idle" connections based on a percentage configured by the user.
2. Do not close the idle connections at all. Keeping connections alive is not expensive on the server.
Please add pros/cons about this in the design and send an email to the community mailing list for a design review/discussion.
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
+ // Close and purge connections from pool if there is more than one being unused
+ if leastUsed != nil && (leastUsed.activeResults() == 0 && connection.activeResults() == 0) {
+ // Close the connection asynchronously since it is a high-latency method
+ go func() {
+ pool.logHandler.log(Debug, closeUnusedPoolConnection)
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }()
+
+ continue
+ }
+
+ // Mark connection as valid to keep
+ pool.connections[validIndex] = connection
+ validIndex++
+
+ // Set the least used connection
+ if leastUsed == nil || connection.activeResults() < leastUsed.activeResults() {
+ leastUsed = connection
+ }
+ } else {
+ pool.logHandler.log(Debug, purgingDeadConnection)
+ }
+ }
+
+ // Deallocate truncated dead connections to prevent memory leak
+ for invalidIndex := validIndex; invalidIndex < len(pool.connections); invalidIndex++ {
+ pool.connections[invalidIndex] = nil
+ }
+ pool.connections = pool.connections[:validIndex]
+
+ // Create new connection if no valid connections were found in the pool or the least used connection exceeded
+ // the concurrent usage threshold while the pool still has capacity for a new connection
+ if leastUsed == nil ||
+ (leastUsed.activeResults() >= pool.newConnectionThreshold && len(pool.connections) < cap(pool.connections)) {
+ return pool.newConnection()
+ } else {
+ return leastUsed, nil
+ }
+ }
+}
+
+func (pool *loadBalancingPool) newConnection() (*connection, error) {
+ connection, err := createConnection(pool.url, pool.logHandler, pool.connSettings)
+ if err != nil {
+ return nil, err
+ }
+ pool.connections = append(pool.connections, connection)
+ return connection, nil
+}
+
+func newLoadBalancingPool(url string, logHandler *logHandler, connSettings *connectionSettings, newConnectionThreshold int,
+ maximumConcurrentConnections int) (connectionPool, error) {
+ pool := make([]*connection, 0, maximumConcurrentConnections)
+ initialConnection, err := createConnection(url, logHandler, connSettings)
+ if err != nil {
+ return nil, err
+ }
+ pool = append(pool, initialConnection)
Review Comment:
Code duplication. Refactor `newConnection()` to work for this use case as well instead?
##########
gremlin-go/driver/connection.go:
##########
@@ -52,7 +60,7 @@ func (connection *connection) errorCallback() {
func (connection *connection) close() error {
if connection.state != established {
- return errors.New("cannot close connection that has already been closed or has not been connected")
+ return newError(err0101ConnectionCloseError)
}
Review Comment:
We need to introduce a 'closing' state here. For the same connection, this method could be called simultaneously from two threads (e.g. one from go routine inside pool.write and another from pool.close). Only one should be allowed to 'close'.
Also please add method doc which mentions whether a function is idempotent or not and also whether it is thread safe or not.
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
+ // Close and purge connections from pool if there is more than one being unused
+ if leastUsed != nil && (leastUsed.activeResults() == 0 && connection.activeResults() == 0) {
+ // Close the connection asynchronously since it is a high-latency method
+ go func() {
+ pool.logHandler.log(Debug, closeUnusedPoolConnection)
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }()
+
+ continue
+ }
+
+ // Mark connection as valid to keep
+ pool.connections[validIndex] = connection
+ validIndex++
+
+ // Set the least used connection
+ if leastUsed == nil || connection.activeResults() < leastUsed.activeResults() {
+ leastUsed = connection
+ }
+ } else {
+ pool.logHandler.log(Debug, purgingDeadConnection)
+ }
+ }
+
+ // Deallocate truncated dead connections to prevent memory leak
Review Comment:
perhaps an attempt to gracefully close them first?
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
+ var leastUsed *connection = nil
+ validIndex := 0
+ for _, connection := range pool.connections {
+ // Purge dead connections from pool
+ if connection.state == established {
Review Comment:
this might end up purging "initialized" state connections too.
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
Review Comment:
what happens when write() is called when another thread is executing the close() method?
##########
gremlin-go/driver/connectionPool.go:
##########
@@ -0,0 +1,147 @@
+/*
+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 (
+ "sync"
+)
+
+type connectionPool interface {
+ write(*request) (ResultSet, error)
+ close()
+}
+
+const defaultNewConnectionThreshold = 4
+
+// loadBalancingPool has two configurations: maximumConcurrentConnections/cap(connections) and newConnectionThreshold.
+// maximumConcurrentConnections denotes the maximum amount of active connections at any given time.
+// newConnectionThreshold specifies the minimum amount of concurrent active traversals on the least used connection
+// which will trigger creation of a new connection if maximumConcurrentConnections has not bee reached.
+// loadBalancingPool will use the least-used connection, and as a part of the process, getLeastUsedConnection(), will
+// remove any unusable connections from the pool and ensure that the returned connection is usable. If there are
+// multiple active connections with no active traversals on them, one will be used and the others will be closed and
+// removed from the pool.
+type loadBalancingPool struct {
+ url string
+ logHandler *logHandler
+ connSettings *connectionSettings
+
+ newConnectionThreshold int
+ connections []*connection
+ loadBalanceLock sync.Mutex
+}
+
+func (pool *loadBalancingPool) close() {
+ for _, connection := range pool.connections {
+ err := connection.close()
+ if err != nil {
+ pool.logHandler.logf(Warning, errorClosingConnection, err.Error())
+ }
+ }
+}
+
+func (pool *loadBalancingPool) write(request *request) (ResultSet, error) {
+ connection, err := pool.getLeastUsedConnection()
+ if err != nil {
+ return nil, err
+ }
+ return connection.write(request)
+}
+
+func (pool *loadBalancingPool) getLeastUsedConnection() (*connection, error) {
+ pool.loadBalanceLock.Lock()
+ defer pool.loadBalanceLock.Unlock()
+ if len(pool.connections) == 0 {
+ return pool.newConnection()
+ } else {
Review Comment:
This else block is not required since there is no code to execute outside the else block.
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#discussion_r851227771
##########
gremlin-go/docker/gremlin-server-integration-secure.yaml:
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
we had managed to reduce the number of copies of these gremlin server config files down to:
* the template one in `gremlin-server/src/test/resources/org/apache/tinkerpp/gremlin/server` - this one is dynamically reconfigured by the groovy server start scripts to produce the three server variations that are tested
* the three configuration files that statically represent the three server variations described above and are stored in `docker/gremlin-server/`
How do we avoid duplicating these here in gremlin-go? and, if gremlin-go is meant to be the model for other GLVs then i think it would be worth including the third configuration even if we dont' support kerberos here.
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto commented on pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619#issuecomment-1110136775
Thanks for reviewing @spmallette and @divijvaidya.
The comments @divijvaidya made are being actively worked on (most have been handled) in another branch.
The comments @spmallette made about the symlink file will be handled here before merging, the comments about docker will be handled separately in this ticket: https://issues.apache.org/jira/browse/TINKERPOP-2737.
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto merged pull request #1619: Gremlin Go MS5 - Feature Complete
Posted by GitBox <gi...@apache.org>.
lyndonbauto merged PR #1619:
URL: https://github.com/apache/tinkerpop/pull/1619
--
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-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org