lidavidm commented on code in PR #4322:
URL: https://github.com/apache/arrow-adbc/pull/4322#discussion_r3277585141
##########
go/adbc/driver/flightsql/flightsql_connection.go:
##########
@@ -222,14 +253,46 @@ var adbcToFlightSQLInfo =
map[adbc.InfoCode]flightsql.SqlInfo{
adbc.InfoVendorSubstraitMaxVersion:
flightsql.SqlInfoFlightSqlServerSubstraitMaxVersion,
}
-func doGet(ctx context.Context, cl *flightsql.Client, endpoint
*flight.FlightEndpoint, clientCache gcache.Cache, opts ...grpc.CallOption) (rdr
*flight.Reader, err error) {
+// doGetWithLogger is the logging-aware DoGet implementation used by every
+// caller in the driver. Earlier revisions exposed a plain `doGet` wrapper
+// that supplied a nil logger; it had no remaining callers and was removed
+// to satisfy the unused-function linter — every call site now passes its
+// own *slog.Logger (or relies on safeLogger to materialize a no-op one).
+//
+// When a logger is provided every endpoint resolution attempt is logged
+// individually so that failures hopping between Flight locations can be
+// traced. The function also accumulates per-location attempt errors and, if
+// every attempt fails, joins them into a single returned error so that the
+// caller can see *all* the locations that were tried (and how each one
+// failed) rather than just the last one. This is critical for diagnosing
+// "[FlightSQL] error reading from server: EOF" reports where the empty
+// "endpoint N: []" indicates the Location list was actually empty: with the
+// enhanced error the operator can see whether multiple alternate locations
+// were attempted and which one ultimately served (or failed) the request.
Review Comment:
We can trim this? e.g. I don't think we need codebase archaeology here.
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -69,6 +71,24 @@ type databaseImpl struct {
options map[string]string
userDialOpts []grpc.DialOption
oauthToken credentials.PerRPCCredentials
+ // tracesExporter records the value of
+ // adbc.OptionKeyTelemetryTracesExporter that was supplied (if any)
+ // at database construction time. The tracer itself is already
+ // initialized by driverbase by the time SetOptions runs, so this
+ // field is retained purely so GetOption can echo back the
+ // configured value (callers expect "get returns what was set") and
+ // so SetOption can return a precise diagnostic when a caller tries
+ // to change the exporter after the database has been opened.
+ tracesExporter string
+ // tracesFolderPath records the value of
+ // adbc.OptionKeyTelemetryTracesFolderPath that was supplied at
+ // construction time. The on-disk RotatingFileWriter behind the
+ // "adbcfile" exporter is created during NewDatabase*, so this field
+ // is retained for symmetry with tracesExporter: GetOption can echo
+ // the configured value back, and SetOption can surface a precise
+ // diagnostic when a caller tries to retarget the folder after the
+ // writer is already running.
+ tracesFolderPath string
Review Comment:
Given the stated purpose, why can't driverbase handle the get/set?
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -504,13 +592,22 @@ func (d *databaseImpl) Open(ctx context.Context)
(adbc.Connection, error) {
var cnxnSupport support
- info, err := cl.GetSqlInfo(ctx,
[]flightsql.SqlInfo{flightsql.SqlInfoFlightSqlServerTransaction}, d.timeout)
+ // Capture the resolved gRPC peer address from the first call so the
Review Comment:
Is this actually stable? I thought there were circumstances under which gRPC
would reconnect, or there may be round-robin DNS load balancing in play, etc.
(Albeit I doubt anyone wants to try to deploy Flight SQL with such a setup...)
--
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]
