CritasWang commented on code in PR #62:
URL:
https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3688065208
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -172,10 +205,61 @@ protected internal SessionPool(List<string> nodeUrls,
string username, string pa
_certificatePath = certificatePath;
_sqlDialect = sqlDialect;
_database = database;
+ _poolWaitTimeoutInMs = poolWaitTimeoutInMs;
+ }
+ /// <summary>
+ /// Acquires a client from the pool. If the pool has no idle client
but owns vacant slots left behind
+ /// by earlier failed reconnections, one of those slots is
re-materialized on the spot instead of
+ /// blocking on a queue that nobody will ever feed. This is what lets
the pool recover on its own
+ /// after the server has been unreachable for a while.
+ /// </summary>
+ private async Task<Client> AcquireClientAsync(CancellationToken
cancellationToken = default)
+ {
+ if (_clients.ClientQueue.IsEmpty && TryReserveVacantSlot())
Review Comment:
You are right, and the documentation I wrote was simply wrong. Fixed in
f855e96.
Of the two options you offered I took the second — **explicitly define this
as demand-driven capacity** — rather than eagerly refilling. Reasoning:
- Eager refill needs either a background thread or a synchronous burst of
`poolSize - 1` connection attempts on the first successful request after an
outage. A background thread is a significant design addition for an SDK that
currently has none, and the synchronous burst would put a latency spike on
exactly the request that just recovered.
- Demand-driven growth is the standard pool semantic (cf. HikariCP's
`minimumIdle` vs `maximumPoolSize`): capacity is a ceiling, not a target. Under
real load the queue does empty, so capacity grows to match demand on its own.
The pathological case you describe — one connection serving everything — is
precisely the case where the extra connections would sit idle anyway.
What changed:
- `_vacantSlots` / `VacantSlots` XML docs now say it measures unrealized
capacity, that a steady non-zero value under light load is normal, and that it
does **not** indicate server availability.
- The metrics table entry changed from "Alert if > 0 and not returning to 0"
to "Not an alert signal on its own".
- The section formerly titled "Self-healing behaviour" is now "Capacity is
demand-driven" and states outright that refill happens only when an acquisition
finds the idle queue empty, so `VacantSlots` legitimately stays above zero
after full recovery.
- Alerting guidance now points at `FailedReconnections`, which only
increases when a reconnection actually fails and is therefore the correct
reachability signal.
- Scenario 2's symptom list clarifies that `FailedReconnections`, not
`VacantSlots`, is the outage signal.
If you would prefer eager refill after all, or think the name `VacantSlots`
still reads as "something is broken" and should be something like
`UnrealizedCapacity`, I am happy to change it — the rename is cheap right now,
before the property ships.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
