Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

Merged
merged 12 commits into from
Sep 16, 2024
Merged
5 changes: 5 additions & 0 deletions balancer/endpointsharding/endpointsharding.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ func (bw *balancerWrapper) UpdateState(state balancer.State) {
bw.es.updateState()
}

// ParseConfig parses a child config list and returns an LB config to use with
// the endpointsharding balancer.
//
// cfg is expected to be a JSON array of LB policy names + configs as the
// format of the loadBalancingConfig field in ServiceConfig.
func ParseConfig(cfg json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {
return gracefulswitch.ParseConfig(cfg)
}
Expand Down
3 changes: 3 additions & 0 deletions balancer/pickfirst/pickfirst.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,13 @@ func (b *pickfirstBalancer) ResolverError(err error) {
})
}

// Shuffler is an interface for shuffling an address list.
type Shuffler interface {
ShuffleAddressListForTesting(n int, swap func(i, j int))
}

// ShuffleAddressListForTesting pseudo-randomizes the order of addresses. n
// is the number of elements. swap swaps the elements with indexes i and j.
func ShuffleAddressListForTesting(n int, swap func(i, j int)) { rand.Shuffle(n, swap) }

func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState) error {
Expand Down
1 change: 1 addition & 0 deletions credentials/tls/certprovider/pemfile/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
)

const (
// PluginName is the name of the PEM file watcher plugin.
PluginName = "file_watcher"
defaultRefreshInterval = 10 * time.Minute
)
Expand Down
2 changes: 2 additions & 0 deletions examples/features/advancedtls/client/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*
*/

// Binary client is an example client demonstrating use of advancedtls, to set
// up a secure gRPC client connection with various TLS authentication methods.
package main

import (
Expand Down
2 changes: 2 additions & 0 deletions examples/features/advancedtls/server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*
*/

// Binary server is an example client demonstrating how to set up a secure gRPC
// server using advancedtls.
package main

import (
Expand Down
2 changes: 2 additions & 0 deletions internal/balancer/gracefulswitch/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ type lbConfig struct {
childConfig serviceconfig.LoadBalancingConfig
}

// ChildName returns the name of the child balancer of the gracefulswitch
// Balancer.
func ChildName(l serviceconfig.LoadBalancingConfig) string {
return l.(*lbConfig).childBuilder.Name()
}
Expand Down
15 changes: 15 additions & 0 deletions internal/channelz/channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,17 @@ type Channel struct {
// Non-zero traceRefCount means the trace of this channel cannot be deleted.
traceRefCount int32

// ChannelMetrics holds connectivity state, target and call metrics for the
// channel within channelz.
ChannelMetrics ChannelMetrics
}

// Implemented to make Channel implement the Identifier interface used for
// nesting.
func (c *Channel) channelzIdentifier() {}

// String returns a string representation of the Channel, including its parent
// entity and ID.
func (c *Channel) String() string {
if c.Parent == nil {
return fmt.Sprintf("Channel #%d", c.ID)
Expand All @@ -61,24 +65,31 @@ func (c *Channel) id() int64 {
return c.ID
}

// SubChans returns a copy of the map of sub-channels associated with the
// Channel.
func (c *Channel) SubChans() map[int64]string {
db.mu.RLock()
defer db.mu.RUnlock()
return copyMap(c.subChans)
}

// NestedChans returns a copy of the map of nested channels associated with the
// Channel.
func (c *Channel) NestedChans() map[int64]string {
db.mu.RLock()
defer db.mu.RUnlock()
return copyMap(c.nestedChans)
}

// Trace returns a copy of the Channel's trace data.
func (c *Channel) Trace() *ChannelTrace {
db.mu.RLock()
defer db.mu.RUnlock()
return c.trace.copy()
}

// ChannelMetrics holds connectivity state, target and call metrics for the
// channel within channelz.
type ChannelMetrics struct {
// The current connectivity state of the channel.
State atomic.Pointer[connectivity.State]
Expand Down Expand Up @@ -136,12 +147,16 @@ func strFromPointer(s *string) string {
return *s
}

// String returns a string representation of the ChannelMetrics, including its
// state, target, and call metrics.
func (c *ChannelMetrics) String() string {
return fmt.Sprintf("State: %v, Target: %s, CallsStarted: %v, CallsSucceeded: %v, CallsFailed: %v, LastCallStartedTimestamp: %v",
c.State.Load(), strFromPointer(c.Target.Load()), c.CallsStarted.Load(), c.CallsSucceeded.Load(), c.CallsFailed.Load(), c.LastCallStartedTimestamp.Load(),
)
}

// NewChannelMetricForTesting creates a new instance of ChannelMetrics with
// specified initial values for testing purposes.
func NewChannelMetricForTesting(state connectivity.State, target string, started, succeeded, failed, timestamp int64) *ChannelMetrics {
c := &ChannelMetrics{}
c.State.Store(&state)
Expand Down
2 changes: 2 additions & 0 deletions internal/channelz/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ func NewServerMetricsForTesting(started, succeeded, failed, timestamp int64) *Se
return sm
}

// CopyFrom copies the metrics data from the provided ServerMetrics
// instance into the current instance.
Comment on lines +62 to +63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// CopyFrom copies the metrics data from the provided ServerMetrics
// instance into the current instance.
// CopyFrom copies the metrics data from the provided ServerMetrics instance
// into the current instance.

func (sm *ServerMetrics) CopyFrom(o *ServerMetrics) {
sm.CallsStarted.Store(o.CallsStarted.Load())
sm.CallsSucceeded.Store(o.CallsSucceeded.Load())
Expand Down
7 changes: 7 additions & 0 deletions internal/channelz/socket.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,18 @@ type EphemeralSocketMetrics struct {
RemoteFlowControlWindow int64
}

// SocketType represents the type of socket.
type SocketType string

// SocketType can be one of these.
const (
SocketTypeNormal = "NormalSocket"
SocketTypeListen = "ListenSocket"
)

// Socket represents a socket within channelz which includes socket
// metrics and data related to socket activity and provides methods
// for managing and interacting with sockets.
type Socket struct {
Entity
SocketType SocketType
Expand All @@ -100,6 +105,8 @@ type Socket struct {
Security credentials.ChannelzSecurityValue
}

// String returns a string representation of the Socket, including its parent
// entity, socket type, and ID.
func (ls *Socket) String() string {
return fmt.Sprintf("%s %s #%d", ls.Parent, ls.SocketType, ls.ID)
}
Expand Down
2 changes: 2 additions & 0 deletions internal/channelz/subchannel.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,14 @@ func (sc *SubChannel) id() int64 {
return sc.ID
}

// Sockets returns a copy of the sockets map associated with the SubChannel.
func (sc *SubChannel) Sockets() map[int64]string {
db.mu.RLock()
defer db.mu.RUnlock()
return copyMap(sc.sockets)
}

// Trace returns a copy of the ChannelTrace associated with the SubChannel.
func (sc *SubChannel) Trace() *ChannelTrace {
db.mu.RLock()
defer db.mu.RUnlock()
Expand Down
19 changes: 14 additions & 5 deletions internal/channelz/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,21 @@ type TraceEvent struct {
Parent *TraceEvent
}

// ChannelTrace provides tracing information for a channel.
// It tracks various events and metadata related to the channel's lifecycle
// and operations.
Comment on lines +82 to +84
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ChannelTrace provides tracing information for a channel.
// It tracks various events and metadata related to the channel's lifecycle
// and operations.
// ChannelTrace provides tracing information for a channel. It tracks various
// events and metadata related to the channel's lifecycle and operations.

type ChannelTrace struct {
cm *channelMap
clearCalled bool
cm *channelMap
clearCalled bool
// The time when the trace was created.
CreationTime time.Time
EventNum int64
mu sync.Mutex
Events []*traceEvent
// A counter for the number of events recorded in the
// trace.
EventNum int64
mu sync.Mutex
// A slice of traceEvent pointers representing the events recorded for
// this channel.
Events []*traceEvent
Comment on lines +94 to +96
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, should this be unexported? Its type is unexported, which is an anti-pattern. It's probably unused outside this package would be my guess/hope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its being used in channelz/internal/protoconv

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not good. We should export traceEvent, then. If that's too hard to do in this PR, it's fine to leave it for now.

Copy link
Contributor Author

@purnesh42H purnesh42H Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this traceEvent is an internal representation of a single trace event. There is already an exported TraceEvent which is used in clientconn. We will have to give it a different name like TraceEventInternal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a very descriptive choice, but yes, some other name.

Is there any linter that covers unexported types being exposed outside a package, like this case? If so I would like to turn it on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no built-in rule but revive provides framework to write custom rules or we can ask revive contributors for this

}

func (c *ChannelTrace) copy() *ChannelTrace {
Expand Down Expand Up @@ -175,6 +183,7 @@ var refChannelTypeToString = map[RefChannelType]string{
RefNormalSocket: "NormalSocket",
}

// String returns a string representation of the RefChannelType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the linter complain for Stringer method not having a comment? This seems unnecessary to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// String returns a string representation of the RefChannelType
// String returns a string representation of the RefChannelType.

func (r RefChannelType) String() string {
return refChannelTypeToString[r]
}
Expand Down
2 changes: 2 additions & 0 deletions internal/idle/idle.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ func (m *Manager) tryEnterIdleMode() bool {
return true
}

// EnterIdleModeForTesting instructs the channel to enter idle mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very optional:

Looking at https://pkg.go.dev/google.golang.org/grpc/internal/idle#Manager.EnterIdleModeForTesting, should we add more info from tryEnterIdleMode() here?

func (m *Manager) EnterIdleModeForTesting() {
m.tryEnterIdleMode()
}
Expand Down Expand Up @@ -266,6 +267,7 @@ func (m *Manager) isClosed() bool {
return atomic.LoadInt32(&m.closed) == 1
}

// Close stops the timer associated with the Manager, if it exists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we say something like?

Suggested change
// Close stops the timer associated with the Manager, if it exists.
// Close prevents calls to enter/exit idle immediately. This call also
// ensures we are not entering/exiting idle mode.

Stoping the timer seems like a side effect?

func (m *Manager) Close() {
atomic.StoreInt32(&m.closed, 1)

Expand Down
2 changes: 2 additions & 0 deletions internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ var (
// ExitIdleModeForTesting gets the ClientConn to exit IDLE mode.
ExitIdleModeForTesting any // func(*grpc.ClientConn) error

// ChannelzTurnOffForTesting disables the Channelz service for testing
// purposes.
ChannelzTurnOffForTesting func()

// TriggerXDSResourceNotFoundForTesting causes the provided xDS Client to
Expand Down
10 changes: 10 additions & 0 deletions internal/stats/metrics_recorder_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ func verifyLabels(desc *estats.MetricDescriptor, labelsRecv ...string) {
}
}

// RecordInt64Count records the measurement alongside labels on the int
// count associated with the provided handle.
func (l *MetricsRecorderList) RecordInt64Count(handle *estats.Int64CountHandle, incr int64, labels ...string) {
verifyLabels(handle.Descriptor(), labels...)

Expand All @@ -62,6 +64,8 @@ func (l *MetricsRecorderList) RecordInt64Count(handle *estats.Int64CountHandle,
}
}

// RecordFloat64Count records the measurement alongside labels on the float
// count associated with the provided handle.
func (l *MetricsRecorderList) RecordFloat64Count(handle *estats.Float64CountHandle, incr float64, labels ...string) {
verifyLabels(handle.Descriptor(), labels...)

Expand All @@ -70,6 +74,8 @@ func (l *MetricsRecorderList) RecordFloat64Count(handle *estats.Float64CountHand
}
}

// RecordInt64Histo records the measurement alongside labels on the int
// histo associated with the provided handle.
func (l *MetricsRecorderList) RecordInt64Histo(handle *estats.Int64HistoHandle, incr int64, labels ...string) {
verifyLabels(handle.Descriptor(), labels...)

Expand All @@ -78,6 +84,8 @@ func (l *MetricsRecorderList) RecordInt64Histo(handle *estats.Int64HistoHandle,
}
}

// RecordFloat64Histo records the measurement alongside labels on the float
// histo associated with the provided handle.
func (l *MetricsRecorderList) RecordFloat64Histo(handle *estats.Float64HistoHandle, incr float64, labels ...string) {
verifyLabels(handle.Descriptor(), labels...)

Expand All @@ -86,6 +94,8 @@ func (l *MetricsRecorderList) RecordFloat64Histo(handle *estats.Float64HistoHand
}
}

// RecordInt64Gauge records the measurement alongside labels on the int
// gauge associated with the provided handle.
func (l *MetricsRecorderList) RecordInt64Gauge(handle *estats.Int64GaugeHandle, incr int64, labels ...string) {
verifyLabels(handle.Descriptor(), labels...)

Expand Down
Loading