[GitHub] [tinkerpop] FlorianHockmann opened pull request #1077: Tinkerpop 2135 Fix handling of closed idle connections in Gremlin.Net driver

["FlorianHockmann (GitHub)" <gi...@apache.org>]

https://issues.apache.org/jira/browse/TINKERPOP-2135

Closed connections are now removed from the pool. This also required a slight change to the population of the pool as it needs to be able to only populate the pool partially when some connections were removed. The previous assumption was that  the pool is either completely populated or completely empty which doesn't hold true any more.

I changed the underlying data structure that holds the connections from `ConcurrentBag` to `ConcurrectDictionary` because `ConcurrentBag` provides no way to efficiently remove a single element. We would have to take all connections out of the bag and then add all but the closed one back into the bag afterwards. `ConcurrentDictionary` makes this more efficient although it looks a bit strange to use a dictionary if we don't really need any key-value-assignment, but unfortunately there is nothing like a `ConcurrentList` right now.

The second commit here is basically optional. It simply wraps the `ConcurrentBag` in a custom collection that offers the API the `ConnectionPool` needs. Accessing a dictionary while only using the keys resulted in strange looking API calls that could be distracting. Also, the data structure used should not leak into nearly every method in `ConnectionPool`.
If others disagree here, then I can also revert the second commit as it is really only a refactoring.

VOTE +1

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

[GitHub] [tinkerpop] olivertowers commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["olivertowers (GitHub)" <gi...@apache.org>]

Thanks @FlorianHockmann, @jorgebay.

I will open JIRA tickets for the fixes/improvements included in the fork.

Is there a common design/feature set for connection pool handling that all gremlin driver variants are aiming for? Or is this more decided on a per language/variant basis?



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

[GitHub] [tinkerpop] jorgebay commented on issue #1077: TINKERPOP-2135 Fix handling of closed idle connections in Gremlin.Net driver

["jorgebay (GitHub)" <gi...@apache.org>]

> [...] continue these two steps until we have a connection that is below the limit, right?

correct :)

About the `TaskScheduler`, its just a recommendation but it doesn't have to be in scope for this iteration.

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

[GitHub] [tinkerpop] olivertowers commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["olivertowers (GitHub)" <gi...@apache.org>]

I had started working on implementing an improved the connection pool handling for Gremlin.NET, which I based very closely on the java gremlin-driver connection pool implementation.

It attempted to solve the similar areas that this PR is addressing as well as few other key areas:

- Select connections based on least used, then based on last activity, to ensure that activity is shared across the pool (important for Cosmos DB Gremlin API where connections will be timed if there is no request activity).
- Avoid racing request from selecting the same connection.
- Schedule creating new connection when capacity is available. This will done when connection could not be selected from the pool, and can remove a dead connection in order to make room for a new connection.
- Allow requests to wait for a connection to become available if the pool is busy/connections are being created. These will time-out based on a setting.
- If a dead connection is returned by a request, create a replacement connection.
- Only consider the pool closed/service unavailable if the pool is unable to reconnect for some timeout.

I used a `lock`/`List<T>` to manage the actual connection pool (though there are other options that I was considering for this).

I've created a PR on my fork [here](https://github.com/olivertowers/tinkerpop/pull/1).

I was planning on proposing this solution, and I didn't see this PR until today.

Do we think it's possible to merge/reconcile some of these capabilities with this PR?


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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Now that I think of it, maybe it would be safer to replace the `while` with `if` statement, wdyt?

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135 Fix handling of closed idle connections in Gremlin.Net driver

["FlorianHockmann (GitHub)" <gi...@apache.org>]

I just pushed a commit that adds a copy-on-write collection like the one used in the Cassandra driver and also implemented a round-robin mechanism for connection scheduling.
I will try to add some tests here, especially for the new connection type. So, I removed my vote again for now.

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

I pushed a commit to address your two remaining comments, @jorgebay.

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

[GitHub] [tinkerpop] jorgebay commented on issue #1077: Tinkerpop 2135 Fix handling of closed idle connections in Gremlin.Net driver

["jorgebay (GitHub)" <gi...@apache.org>]

It's looking great and its a nice improvement.

I have a couple of suggestions:
- `ConcurrentDictionary.Count` is a [really expensive call](https://github.com/Microsoft/referencesource/blob/master/Microsoft.Bcl/System.Threading.Tasks.v1.5/System/Collections/Concurrent/ConcurrentDictionary.cs#L830): Can we use copy-on-write collections? I've implemented one here, if you'd like to reuse/adapt: https://github.com/datastax/csharp-driver/blob/master/src/Cassandra/Collections/CopyOnWriteList.cs
- 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. That way we make sure nothing is occurring on another thread that can modify the internal state.

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

[GitHub] [tinkerpop] jorgebay commented on issue #1077: TINKERPOP-2135 Fix handling of closed idle connections in Gremlin.Net driver

["jorgebay (GitHub)" <gi...@apache.org>]

> What's the advantage of such a custom copy-on-write list?

Any read operation is cheap and the iterator is not affected by concurrent modifications (unlike concurrent collections). Effectively what you get is a moment-in-time view of the collection, you don't have to add special conditions to track concurrent changes.

About using a queue, IMO we shouldn't rely on that sort of behaviour here (I think we discussed a similar queue approach for the pool in https://github.com/apache/tinkerpop/pull/903):
- We should support concurrent writes to the connection, if we queue it and dequeue it, we might end up with an empty pool (at a certain time). We might need complicated logic to guard for that.
- Using a enqueuing/dequeueing from a concurrent queue is not a cheap operation when multiple threads are accessing to it. We will be introducing unnecessary overhead.
- We shouldn't iterate through all the connections when borrowing from the pool, we just need to round robin through them, sample pseudo code: 

```C#
// obtain something that is fixed for this small period of time
IList<Connection> immutableCopyOfConnections = ... 
int counter = Interlocked.Increment(ref _sharedCounter);
int index = counter % immutableCopyOfConnections.Count;
return immutableCopyOfConnections[index];
```

If we need to compare the one with least in-flight requests, we should minimize the amount of checks, for example check connections at `index` and `index+1`, instead of checking the whole list (`O(2)` vs `O(n)`).

> Do you know whether the default `TaskScheduler` would work here?

No, we will need a custom one like `LimitedConcurrencyLevelTaskScheduler`.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Note that, if there are 2 or more connections being created and one creation fails, the other connection will be open but not returned / or added to the `_connections` list, so it will be leaked.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

This can be the condition of the `while` statement.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

I'm thinking on situations when you end up with a half backed pool, that can be because initially its not possible to create all connections or because one of the connections in the pool closes, after the successfully establishing it.

> Maybe a try-finally where we just Dispose() all createdConnections in the finally block?

Sounds good. I don't have an strong opinion on how to handle it, however you decide is fine by me, as long as we have a consistent behaviour.

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

> If there is another connection open, I would prefer EnsurePoolIsPopulatedAsync() to occur in the background

That is what you suggested with the task scheduler, right? (Maybe I'm just understanding you wrong here, I'm especially not sure about how a back-off delay could work.) I agree that doing this in the background makes more sense in this case, but I won't be able to implement this in time for 3.4.1 and I also prefer not to change too much at the same time if it's not necessary.
So, I would like to just create a ticket for this issue and then try to implement it for 3.4.2.

Adding a delay and a counter here to the loop would be easy to implement. So, if you think that's already an improvement then I can add that.

> fail as fast as possible.

That should be the current situation as we don't catch any exception here. So it fails when one connection establishment fails.

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

[GitHub] [tinkerpop] FlorianHockmann closed pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

[ pull request closed by FlorianHockmann ]

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Do you think that that is really a problem? I expect the check to succeed for the first connection already in like 99,9% of the cases because thanks to the round-robin we should have an equal distribution of requests over connections and if the connections actually start reaching this limit, then it won't take long until a `ConnectionPoolBusyException` is thrown at which case users have to increase the limits any way which will make the situation again less likely to occur.
That's why I also don't see the benefit in limiting the number of checks here. It would just reduce the time until an exception is thrown minimally, but it probably won't result in performance benefits for 99,9% of the requests.

I can add a dedicated counter though to the `Connection` object if you want to. Just wanted to better understand your reasoning here.

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135 Fix handling of closed idle connections in Gremlin.Net driver

["FlorianHockmann (GitHub)" <gi...@apache.org>]

>  the iterator is not affected by concurrent modifications (unlike concurrent collections)

This seems to depend on the collection actually. For `ConcurrentDictionary` it seems to include modifications, but for `ConcurrentQueue` not ([Source](https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentqueue-1.getenumerator?view=netframework-4.7.2#remarks)):

> The enumeration represents a moment-in-time snapshot of the contents of the queue. It does not reflect any updates to the collection after GetEnumerator was called. The enumerator is safe to use concurrently with reads from and writes to the queue.

I understand your reasoning though for a custom copy-on-write list now like the one you linked in the DataStax Cassandra driver.
Your suggestion of replacing the current `SelectUsedConnection` method with such a round-robin mechanism also makes sense to me. We would get the same benefit as with the enqueue/dequeue mechanism but without the disadvantages you mentioned (possibility to get an empty pool and more expensive operations).
You already suggested that in #903, but I wanted to avoid adding too much complexity there at once and the approach in `SelectUsedConnection` seemed a lot simpler to me at that time. But now that we have a first version of request pipelining working, I think that it makes sense to add this now as an incremental improvement.

> If we need to compare the one with least in-flight requests, we should minimize the amount of checks, for example check connections at index and index+1, instead of checking the whole list (O(2) vs O(n))

Just to make sure that I understand your proposal here correctly: You mean that we would just select the next connection via round-robin and then verify whether it has less than the maximum number of connections and continue these two steps until we have a connection that is below the limit, right? That should usually finish in the first iteration I guess.

Thanks for your suggestions here again so far, @jorgebay! I will give this a try and then update this PR.

I'm just not sure about the task scheduler yet. That sounds like a good idea, but it also adds quite some complexity. So it could make sense to postpone that for 3.4.2. Or do you see that as a must-have already for this version?

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Good catch. Maybe a try-finally where we just `Dispose()` all `createdConnections` in the `finally` block?

I think if a connection establishment fails then we should just let the exception be raised to the user as we can't really handle that.
(Maybe it makes sense later to add a retry here, but we should probably have a logger for cases like that so users can notice such problems.)

Or what do you think?

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

[GitHub] [tinkerpop] spmallette commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["spmallette (GitHub)" <gi...@apache.org>]

VOTE +1 - mostly on principle - you guys have beat this one up pretty well. i doubt i can add much more technically. please remember to update CHANGELOG on merge.

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Hi @olivertowers, first of all, thanks for working on improving Gremlin.NET!

I'm not sure if you're aware of this but we are currently preparing for the release of 3.3.6 and 3.4.1 and entered code freeze on Friday. This means that we now only make small changes and mainly keep the release branches as is so they can be reviewed and tested for the release. We can make exceptions to this policy for very small changes or for pull requests that were already open on Friday and which we can close shortly.
Based on this, I think that our options here are:

1. Combine both pull requests, but merge them only after the 3.4.1 release since your PR is too big of a change to be merged during code freeze.
2. Merge this PR without bigger modifications and apply other changes after the 3.4.1 release with follow-up PRs.

I tend towards option 2 here, because I would like to close TINKERPOP-2135 and TINKERPOP-2148 for this release and I don't think that we can make bigger changes to this PR and still merge it during code freeze.

In general, I think it would good if you could split your PR into multiple smaller ones that address ideally only one issue each. Some of the improvements you suggest are obvious improvements, but others will probably require some discussion. For example, [we decided earlier](https://github.com/apache/tinkerpop/pull/903#issuecomment-420292652) that we don't want to wait for a connection to become available with a timeout and instead fail fast by throwing an exception if no connection is available. I'm not saying that we have to stick with this decision if you have good arguments against it, but this will require some discussion.
If you [create issues in JIRA](https://issues.apache.org/jira/projects/TINKERPOP/issues) first for these issues that you want to fix/improve, then we can also discuss about them first in general before you put in the work to create a dedicated PR for the issue.
I think that it would be easier and more efficient to apply your improvements like this, but we could of course also review your PR at once if you prefer it that way. That will however most likely result in a big review with quite a few iterations until it's in a state where we can merge it.

@jorgebay: Do you agree?

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Anyone available to review this today?
If not, then we can also merge this for 3.4.2 instead. Both issues addressed here aren't critical so I don't want to rush anyone.

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Hmm, yes, kind of obvious now that you mention it.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

oh, nvm, I was thinking you were comparing connections with least in-flights and loop through all connections... sorry.

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

So, a counter that breaks out of the loop after like 10 retries and `Task.Delay(TimeSpan.FromMilliseconds(5))` before the `continue`?

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

I think we should implement some kind of back-off or delayed retry. Otherwise, if there is a failure, this loop will never stop until we get it to the pool gets to the final length.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Note that `NrRequestsInFlight` uses `ConcurrentDictionary.Count` which is super expensive.

We can maintain a separate counter or maybe limit the comparison to 2 connections max for now (and fix later): `for (...; i < Math.Min(connections.Length, 2); ...)`

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

> Is there a common design/feature set for connection pool handling that all gremlin driver variants are aiming for? Or is this more decided on a per language/variant basis?

It's more the latter. Since C# is in general quite similar to Java and because the Java driver was the first one and is probably still the most advanced one, we often use that driver as kind of a reference for the .NET driver. But on the other hand we also don't copy everything from the Java driver. I especially tried to avoid some of the complexities of the Java driver where I didn't see a need yet to introduce them.
So, long story short: The development of the drivers is independent for the different languages, but where it makes sense to copy the Java driver, we do that to make it easier to switch between languages, especially for contributors.

@jorgebay Will you have the time to review this again?

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

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: Tinkerpop 2135 Fix handling of closed idle connections in Gremlin.Net driver

["FlorianHockmann (GitHub)" <gi...@apache.org>]

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 dev@tinkerpop.apache.org

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

Just added tests for the collection type which required to add `InternalsVisibleTo` as I didn't want to make the collection `public`, but that was probably a good idea in general as we can now also test other internal classes in the future more easily.
My VOTE is now again +1 (pending a successful Travis build, the tests worked locally) and the PR is ready for review again.

The Cassandra C# driver was really helpful here. The copy-on-write collection and the round-robin scheduling work now basically the same way as there.

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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Depends on how we want to handle pool failures.

>From what I understand, `GetAvailableConnectionAsync()` gets called every time, and it calls `EnsurePoolIsPopulatedAsync()` so, it's possible that during the lifetime of the driver, a connection is closed and `EnsurePoolIsPopulatedAsync()` needs to create a new one:
- If there is another connection open, I would prefer `EnsurePoolIsPopulatedAsync()` to occur in the background, with some kind of back-off delay (in the order of hundreds of millis, seconds or we can use exponential back off).
- If there isn't another open connection, we can await until one connection is open and fail as fast as possible.

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

[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["FlorianHockmann (GitHub)" <gi...@apache.org>]

It's probably clearer like that, yes. With an `if` it won't really work any more when the pool is completely empty and two requests come in in parallel, but letting the second request loop until the first has populated the pool is probably not a really good idea in general. With an `if` we will just fail fast here.
Until we have a mechanism to create connections in the background again with a timer or something like that, failing fast is probably the better and safer option here.

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

[GitHub] [tinkerpop] jorgebay commented on issue #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Looking forward to include your contributions @olivertowers , welcome!

I agree with @FlorianHockmann that we should first try to land this one, which scope is defined in the tickets linked above and we have been implementing/reviewing for some time now.

Discussing stuff in JIRA / mailing list is sometimes a pain in the ass but it's the way we use to sync different ideas / approaches, I encourage you to create tickets / mailing list threads before submitting code, check out the doc for more info: http://tinkerpop.apache.org/docs/current/dev/developer/#_contributing



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

[GitHub] [tinkerpop] jorgebay commented on pull request #1077: TINKERPOP-2135/2148 Fix removal of closed connections and add round-robin scheduling

["jorgebay (GitHub)" <gi...@apache.org>]

Nit: we could use a normal return statement instead of `out`.

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