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

feat: add AZ to replica info #721

Merged
merged 8 commits into from
Jan 18, 2025
Merged

feat: add AZ to replica info #721

merged 8 commits into from
Jan 18, 2025

Conversation

proost
Copy link
Contributor

@proost proost commented Jan 12, 2025

previous discussion: #710

pipe.go Outdated Show resolved Hide resolved
pipe.go Outdated Show resolved Hide resolved
cluster.go Outdated
primaryConn := c.connFn(primary, c.opt)
conns[primary] = connrole{conn: primaryConn}
if len(g.nodes) > 0 {
g.nodes[0].AZ = primaryConn.AZ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the AZ() is not yet available right after c.connFn. c.connFn only constructs the *mux without making the actual TCP connection. We may need another way to do this.

Copy link
Collaborator

@rueian rueian Jan 13, 2025

Choose a reason for hiding this comment

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

Oh, I just saw you implemented AZ() as m.pipe(0).AZ() then it would make an actual connection to the server.
But here is still not a good place to call AZ(). It will be better if we call AZ() only when the AZ_AFFINITY is enabled and call it at

rIndex := c.opt.ReplicaSelector(uint16(i), g.nodes[1:])
with batching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you mean not always set AZ to ReplicaInfo but if user want to know, then set AZ to ReplicaInfo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the AZ() should be invoked only when necessary, which is when the AZ_AFFINITY is configured.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And, the current calling point, which is right after result.parse, is not a good place to call AZ(). It will always create new connections. We should instead call it after:

rueidis/cluster.go

Lines 232 to 243 in 09ad427

var removes []conn
c.mu.RLock()
for addr, cc := range c.conns {
if fresh, ok := conns[addr]; ok {
fresh.conn = cc.conn
conns[addr] = fresh
} else {
removes = append(removes, cc.conn)
}
}
c.mu.RUnlock()

So that we can reuse existing connections as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@proost proost requested a review from rueian January 13, 2025 14:58
pipe.go Outdated Show resolved Hide resolved
@proost proost requested a review from rueian January 15, 2025 14:53
cluster.go Outdated
@@ -259,6 +259,10 @@ func (c *clusterClient) _refresh() (err error) {
}
if len(g.nodes) > 1 {
n := len(g.nodes) - 1
for i, replica := range g.nodes[1:] {
rConn := conns[replica.Addr].conn
g.nodes[i+1].AZ = rConn.AZ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the others look good to me. The only thing left is that AZ() itself is too costly. Calling AZ() sequentially could block the refresh for a long time if all the rConn haven't actually connected.

I think we should continue to complete the AZ_AFFINITY feature in this PR so that we can skip AZ() calls when it is not configured on the client. Parallelizing AZ() calls can be another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

462f2a9

I'm perfectly wrong. How about this?

@proost proost requested a review from rueian January 16, 2025 08:59
cluster.go Outdated Show resolved Hide resolved
cluster.go Show resolved Hide resolved
rueidis.go Outdated Show resolved Hide resolved
@proost proost requested a review from rueian January 18, 2025 07:20
Copy link
Collaborator

@rueian rueian left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @proost!

@rueian rueian merged commit f14b451 into redis:main Jan 18, 2025
30 checks passed
@proost proost deleted the feat-az-affinity branch January 19, 2025 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants