-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Tablet Manager: Make WaitForPosition() accept either file:pos or GTID-based positions. #6374
Tablet Manager: Make WaitForPosition() accept either file:pos or GTID-based positions. #6374
Conversation
conn, err = mysql.Connect(ctx, params) | ||
if err != nil { | ||
return err | ||
} |
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 there a need to conn.Recycle()
?
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.
For mysql.Connect
there is no Recycle()
. Recycle is a method available to a connection coming from a connection pool. In this case, we are making a direct connection to override the flavor.
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.
It sounds like this code is going away anyway, but for future reference, we would need to defer conn.Close()
in this case.
…nnection. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
5a42866
to
7d81833
Compare
Refactored per @enisoc's suggestions. PTAL when you have the chance, thanks! |
if err != nil { | ||
return Position{}, err | ||
} | ||
return Position{ | ||
GTIDSet: gtidSet, | ||
}, nil |
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.
Merely a coding style comment, feel free to ignore. What I usually do is:
if err != nil { | |
return Position{}, err | |
} | |
return Position{ | |
GTIDSet: gtidSet, | |
}, nil | |
return Position{ | |
GTIDSet: gtidSet, | |
}, err |
it's the caller's responsibility to check if err != nil
and if so, to ignore the GTIDSet
value.
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.
I followed the pattern established in the rest of the codebase but you bring up a good point. I'm somewhat split on this. I agree with you that it's the caller's responsibility to check if err != nil
but I'm also used to writing in languages where if a result is errorful, it's not even possible to access the successful result.
In this case though if err is not nil, then gtidSet would be nil anyway, so I think your version would actually return the same result with less code - and I like that. I suppose it's a question of how others feel regarding established patterns that already exist in the Vitess codebase, and if we should follow those patterns or break them when it's sensible? @enisoc @deepthi @systay @sougou?
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.
I prefer the less compact version because it is easier to read.
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.
LGTM
This PR modifies
WaitMasterPos
which is ultimately called byWaitForPosition()
such thatWaitMasterPos()
will respect the flavor of theGTIDSet
in the suppliedPosition
if that flavor isfilePos
. Previously, no matter what flavor thePosition
was, we always deferred to the flavor inherent to the connection grabbed from theMysqld
connection pool. This PR provides an essential update that allowsWaitForPosition()
to accept either file-based positions, or GTID based positions.Related Issue: #6206