-
Notifications
You must be signed in to change notification settings - Fork 7
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
transport: use go 1.19 atomic.Pointer instead of atomic.Value #284
base: main
Are you sure you want to change the base?
Conversation
5fa174f
to
bd3757b
Compare
9235bba
to
15058fa
Compare
transport/cluster.go
Outdated
@@ -443,7 +443,7 @@ func parseTokensFromRow(n *Node, r frame.Row, ring *Ring) error { | |||
} | |||
|
|||
func (c *Cluster) Topology() *topology { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Topology()
method still necessary? It is an exported method returning non-exported type, so I wonder if we could just remove it in favor of using c.topology.Load()
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topology()
is used in session to get all nodes to prepare a query on them. Nodes are the only field of topology that is currently exported and used outside of transport
, so I suppose Topology()
could be removed and replaced with just func (*Cluster) Nodes()
.
The other thing is that topology is passed inside QueryInfo
to host selection policies, but same as all fields of QueryInfo
, it is unexported. It likely should be accessible outside of transport
, opened #287 about it, it may influence if topology will be exported.
The point of Topology() and setTopology() cluster methods was to give some sort of type safety as topology was an atomic.Value that can hold any type. As topology is now an atomic.Pointer they are no longer needed. Session sometimes needs to perform a query on all nodes of the cluster, to allow that cluster now has an exported Nodes() method.
15058fa
to
f757a77
Compare
Currently transport package atomically replaces topology and connections with new ones. This is done using atomic.Value that hold topology and conn pointers, this is not type-safe and requires type assertions every time the driver needs the values hidden in atomic.Value.
Go 1.19 introduces a generic atomic.Pointer[T], that is a more suitable replacement, adding type-safety and removing the type assertions,
This PR replaces mentioned atomic.Values with the new generic atomic pointer type.
Fixes #269