Update VReplication Timestamp w/ Heartbeat#6635
Conversation
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
… a workflow. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…ensuring that transaction_timestamp stays up to date. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…esn't get recorded. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…ed, which allows us to scrub incoming queries for heartbeat updates. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
PrismaPhonic
force-pushed
the
update-transaction-with-heartbeat
branch
2 times, most recently
from
a47c176 to
a134bb8
Compare
PrismaPhonic
requested review from
deepthi,
enisoc,
rohit-nayak-ps,
shlomi-noach and
systay
create unpredictable results for unit testing. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
PrismaPhonic
force-pushed
the
update-transaction-with-heartbeat
branch
from
a134bb8 to
68b0d74
Compare
| continue | ||
| } | ||
| var got string | ||
| heartbeatRe := regexp.MustCompile(`update _vt.vreplication set time_updated=\d+, transaction_timestamp=\d+ where id=\d+`) |
There was a problem hiding this comment.
possibly make the query case insensitive? Granted, _vt.vreplication is case sensitive, but all other elements, keywords & columns, are not.
| heartbeatRe := regexp.MustCompile(`update _vt.vreplication set time_updated=\d+, transaction_timestamp=\d+ where id=\d+`) | |
| heartbeatRe := regexp.MustCompile(`(?i)update _vt.vreplication set time_updated=\d+, transaction_timestamp=\d+ where id=\d+`) |
There was a problem hiding this comment.
In this case we are the ones writing the heartbeat queries and they are always written as lower case. If there actually was a query written where that wasn't the case, it would be a sure sign that it was not a heartbeat update query. Therefore, I think it being case sensitive is a better catch method.
| case got = <-globalDBQueries: | ||
|
|
||
| if got == "begin" || got == "commit" || got == "rollback" || strings.Contains(got, "update _vt.vreplication set pos") { | ||
| if got == "begin" || got == "commit" || got == "rollback" || strings.Contains(got, "update _vt.vreplication set pos") || heartbeatRe.MatchString(got) { |
There was a problem hiding this comment.
perhaps the query gets lower cased beforehand? Otherwise, BEGIN, COMMIT, ROLLBACK should also be accepted.
There was a problem hiding this comment.
These were already here so I assume whoever built this had some wisdom with writing this catch. I believe that we always write them as lowercase (in vplayer), so this is safe. It's also just testing logic so if it breaks at some point, whoever broke it (by changing the casing that vplayer writes queries with) could adjust it then to fit their needs.
| case got = <-globalDBQueries: | ||
|
|
||
| if got == "begin" || got == "commit" || got == "rollback" || strings.Contains(got, "update _vt.vreplication set pos") { | ||
| if got == "begin" || got == "commit" || got == "rollback" || strings.Contains(got, "update _vt.vreplication set pos") || heartbeatRe.MatchString(got) { |
There was a problem hiding this comment.
Is there any risk at all for some query to contain the text update _vt.vreplication set pos? Does it make sense to convert strings.Contains() to strings.HasPrefix()?
There was a problem hiding this comment.
That's a good question. I'm not sure why the original author of this test helper used Contains. It might be in an effort to strip potential whitespace? Or potentially to strip leading / which somehow end up in some of the test queries at the start?
6 participants
This PR:
_vt.vreplicationrow timestamp w/ each heartbeat that happens outside of a transaction. This improves the accuracy of the transaction_timestamp recorded for each_vt.vreplicationrow, and allows anyone inspecting the_vt.vreplicationtable to get an accurate read on replication lag.Workflow Showsubcommand, which displays the maximum vreplication lag seen across all shards.