Thanks for your review, @jorgebay!

I'm not sure if you're following on 
[TINKERPOP-2148](https://issues.apache.org/jira/browse/TINKERPOP-2148), but 
user Zaoshi suggested there to switch from `ConcurrentBag` to 
`ConcurrentQueue`. This would have two nice advantages for us right now:

1. We don't have to iterate over all connections and find the least used one. 
Instead, we can simply dequeue/enqueue connections and use the first that 
hasn't reached it's limit. This is effectively a round-robin mechanism which is 
probably more efficient than our current approach.
2. Removal of a closed connection would be as simple as not enqueueing it again 
during this process.

Would be good to hear your opinion on that.

What's the advantage of such a custom copy-on-write list? Only avoiding the 
expensive `Count` implementation of `ConcurrentDictionary`? That seems to be at 
least less expensive for 
[`ConcurrentQueue`](https://github.com/Microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/mscorlib/system/collections/Concurrent/ConcurrentQueue.cs#L394).
 But we would probably have to maintain our own connection counter if we moved 
to such a dequeue/enqueue approach as that would change the count all the time.

In general, I would prefer if we could stick with the default collection types 
if possible and only add our own types if we really have to.

> To ensure that concurrent modifications to the pool don't occur, e.g., pool 
> growing and closing, we could use something like a ordered task scheduler.

I also thought about how we could prevent that, but I wanted to postpone it to 
a subsequent PR. A task scheduler sounds like a good solution in general. Do 
you know whether the default 
[TaskScheduler](https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskscheduler)
 would work here? I haven't worked with that yet myself.
With a task scheduler we would probably only have to change the way we replace 
dead connections. Currently, all connections are removed from the pool if a 
request fails due to a network error and new connections are created the next 
time a connection is needed. That would not work anymore if a scheduled task is 
used to create new connections as we probably can't really wait for that (?)

[ Full content available at: https://github.com/apache/tinkerpop/pull/1077 ]
This message was relayed via gitbox.apache.org for [email protected]

Reply via email to