hubcio commented on code in PR #3379:
URL: https://github.com/apache/iggy/pull/3379#discussion_r3379500900
##########
foreign/go/internal/util/leader_aware.go:
##########
@@ -82,14 +96,23 @@ func processClusterMetadata(metadata
*iggcon.ClusterMetadata, currentAddress str
}
leaderAddress := net.JoinHostPort(leader.IP,
strconv.Itoa(int(leaderPort)))
- log.Printf("Found leader node: %s at %s (using %s transport)\n",
leader.Name, leaderAddress, transport)
+ logger.Info(
Review Comment:
`logger.Info("Found leader node", ...)` fires before the `isSameAddress`
check below, so it logs at info on every redirect check even when you are
already on the leader and nothing changes. the steady-state already-leader case
probably wants debug here, matching the `Debug` line at the bottom of the
function.
##########
foreign/go/internal/util/leader_aware.go:
##########
@@ -82,14 +96,23 @@ func processClusterMetadata(metadata
*iggcon.ClusterMetadata, currentAddress str
}
leaderAddress := net.JoinHostPort(leader.IP,
strconv.Itoa(int(leaderPort)))
- log.Printf("Found leader node: %s at %s (using %s transport)\n",
leader.Name, leaderAddress, transport)
+ logger.Info(
+ "Found leader node",
+ "leader", leader.Name,
+ "address", leaderAddress,
+ "transport", transport,
+ )
if !isSameAddress(currentAddress, leaderAddress) {
- log.Printf("Current connection to %s is not the leader, will
redirect to %s\n", currentAddress, leaderAddress)
+ logger.Info(
+ "Current connection is not the leader, redirecting",
+ "current_address", currentAddress,
+ "leader_address", leaderAddress,
+ )
return leaderAddress, nil
}
- log.Printf("Already connected to leader at %s\n", currentAddress)
+ logger.Debug("Already connected to leader at", "current_address",
currentAddress)
Review Comment:
message reads `"Already connected to leader at"` with the address now moved
to the `current_address` attr, so it renders with a dangling "at". drop the
trailing word - `"Already connected to leader"`. the redirect log just above
was reworded fine, this one got missed.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -195,7 +197,7 @@ func WithTLSValidateCertificate(validate bool) TLSOption {
// NewIggyTcpClient creates a new Iggy TCP client with the given options.
// warning: don't use this function directly, use iggycli.NewIggyClient with
iggycli.WithTcp instead.
-func NewIggyTcpClient(options ...Option) *IggyTcpClient {
+func NewIggyTcpClient(logger *slog.Logger, options ...Option) *IggyTcpClient {
Review Comment:
`NewIggyTcpClient` takes `logger *slog.Logger` as a required positional arg
but stores it raw with no nil-guard. calling `NewIggyTcpClient(nil)` (or a bare
`IggyTcpClient{}` literal) leaves the field nil, and the first login runs
`HandleLeaderRedirection` -> `CheckAndRedirectToLeader`, whose first statement
is `logger.Debug(...)` - nil pointer deref panic, no recover. the documented
`NewIggyClient` + `WithLogger` path is safe (default `DiscardHandler` plus the
nil-guard in `WithLogger`), so this only bites direct callers of this exported
ctor - but it is exported, and the new required logger arg makes `nil` the
natural no-logging value.
cheapest fix is one chokepoint here: `if logger == nil { logger =
slog.New(slog.DiscardHandler) }`. that covers every `logger.Debug/Info/Warn` in
`leader_aware.go` at once (several of them deref unconditionally, so guarding
only the first call site would not be enough). keep the `WithLogger` guard too
- it protects the separate `ic.logger` heartbeat field, which this ctor never
touches.
separately, the signature change from `(options ...Option)` is a compile
break for any external code calling this directly - fine since it is
doc-discouraged and pre-1.0, but worth a changelog note.
##########
foreign/go/client/iggy_client.go:
##########
@@ -54,8 +56,22 @@ func WithTcp(tcpOpts ...tcp.Option) Option {
}
}
+// WithLogger sets the logger for the Iggy client and its underlying transport.
+// This logger is used by the heartbeat and forwarded to the transport as a
Review Comment:
doc says the logger is "forwarded to the transport as a default", but there
is no `tcp.WithLogger` override anymore (it was removed in this branch), so "as
a default" implies an override path that no longer exists. the logger is just
passed straight to the transport - reword to drop "as a default".
##########
foreign/go/internal/util/leader_aware.go:
##########
@@ -82,14 +96,23 @@ func processClusterMetadata(metadata
*iggcon.ClusterMetadata, currentAddress str
}
leaderAddress := net.JoinHostPort(leader.IP,
strconv.Itoa(int(leaderPort)))
- log.Printf("Found leader node: %s at %s (using %s transport)\n",
leader.Name, leaderAddress, transport)
+ logger.Info(
+ "Found leader node",
+ "leader", leader.Name,
+ "address", leaderAddress,
Review Comment:
`leaderAddress` is logged under key `"address"` here but as
`"leader_address"` a few lines down (line 110), same value two keys. pick one
so the structured keys stay consistent.
--
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]
