REP-6804 Add option to read oplog directly #174
Conversation
tdq45gj
left a comment
tdq45gj
left a comment
There was a problem hiding this comment.
I'm not familiar with the oplog tailing logic, but it looks good to me overall. I've left some small comments.
| ctx context.Context, | ||
| client *mongo.Client, | ||
| ) (OpTime, OpTime, error) { | ||
| oldestTxn, err := getOldestTransactionTime(ctx, client) |
There was a problem hiding this comment.
Is there a ticket to track swapping the order of getOldestTransactionTime() and getLatestVisibleOplogOpTime() per the issue described in TOOLS-4015?
There was a problem hiding this comment.
🤦 This is brand-new logic … I’m not sure why I wrote it this way. Fixed, and thank you.
README.md
Outdated
| ## `tailOplog` | ||
| The verifier will read the oplog continually instead of reading a change stream. This is generally faster, but it doesn’t work in sharded clusters. |
There was a problem hiding this comment.
Should we change "doesn't work" to be "requires connecting directly to each shard"?
There was a problem hiding this comment.
I’ve been under the impression that direct shard connections are something we want people not to do. But, revised.
internal/verifier/oplog_reader.go
Outdated
| } | ||
|
|
||
| func getOplogDocIDExpr(docroot string) any { | ||
| // $switch was new in MongoDB 4.4, so use $cond instead. |
There was a problem hiding this comment.
$switch was added in MongoDB 3.4 (SERVER-10689). Can this comment be removed?
There was a problem hiding this comment.
I must have misread 3.4 as 4.4 … of course it doesn’t matter because the expr form only works in 4.4+. Removed.
| // op=c is for applyOps, and also to detect forbidden DDL. | ||
| agg.And{ | ||
| agg.Eq{"$op", "c"}, | ||
| agg.Not{helpers.StringHasPrefix{ |
There was a problem hiding this comment.
[question] Do we limit ourselves to ignoring DDL operations on the "config" database because it would be more complex MQL to also ignore DDL operations on the "admin" database? I don't anticipate op=c oplog entries on the "config" database after a cluster is first initialized so I wanted to double check the intention here (I'm not proposing any changes).
There was a problem hiding this comment.
I believe I copied this part from mongo-speedcam, which I essentially “reverse-engineered” from mongomirror.
The ideal, actually, is to detect & fail on any DDL event. In that regard, would it be better to remove the filtering on config here so we catch DDL events on config?
| return nil, bson.Timestamp{}, errors.Wrapf(err, "extracting applyOps[%d].op", i) | ||
| } | ||
|
|
||
| err = parseOneDocumentOp(opName, latestTS, opRaw) |
There was a problem hiding this comment.
[note] I imagine because this logic is only used by the verifier it is ok for rechecks to be spuriously performed on documents which are attempted to be modified by a multi-statement transaction which ultimately aborts. As a comparison point, change streams only emits change event for multi-statement transactions after they are committed.
This behavior of the oplog reader is also what enables its resume token to only be aware of the 'ts' timestamp position within the oplog collection.
I'm unsure if this is the best spot to call this out or if there's some high-level overview on how the oplog reader mode behaves it can be summarized in instead.
There was a problem hiding this comment.
I added a note at the top of this file about the 2 cases (including this) where oplog reader triggers “extra” rechecks.
3 participants
This adds an opt-in mode to tail the oplog directly instead of reading a change stream. Oplog tailing is less resource-intensive on the server than a change stream, which yields better performance and allows verification of more migrations.
Oplog tailing carries a few caveats (or else it would be the default when possible):
Additional notes:
agglibrary, which simplifies aggregation usage in Go by providing types for various expressions.