feat: schema watch support for mysql driver #2224
Conversation
Signed-off-by: Kartikay <[email protected]>
github-actions
bot
added
area/datastore
|
@josephschorr I thought this PR would need refactoring after the changes to base of datastore driver, after looking closely it seems that this could go ahead as well, my bad. |
internal/datastore/mysql/watch.go
Outdated
|
|
||
| // Load caveat changes for the revision range. | ||
| if options.Content&datastore.WatchSchema == datastore.WatchSchema { | ||
| if err := mds.loadCaveatChanges(ctx, afterRevision, newRevision, stagedChanges); err != nil { |
There was a problem hiding this comment.
Put this under the if statement above
internal/datastore/mysql/watch.go
Outdated
| return fmt.Errorf("unable to parse changed namespace: %w", err) | ||
| } | ||
|
|
||
| if createdTxn > afterRevision && createdTxn <= newRevision { |
There was a problem hiding this comment.
why the filter here if we're doing so in the SQL query?
There was a problem hiding this comment.
The query can return a wider net of rows in the watch window, hence a double check, was implemented earlier for relationships too, also tested without the if statement, datastore tests tend to break in that case.
internal/datastore/mysql/watch.go
Outdated
|
|
||
| func (mds *Datastore) loadCaveatChanges(ctx context.Context, afterRevision uint64, newRevision uint64, stagedChanges *common.Changes[revisions.TransactionIDRevision, uint64]) (err error) { | ||
| sql, args, err := mds.QueryChangedCaveatsQuery.Where(sq.Or{ | ||
| sq.And{ |
There was a problem hiding this comment.
We should be able to combine these methods
There was a problem hiding this comment.
done, should I do same for loadRelationships and combine that too? (right now loadSchemaChanges is only called when schema watch is enabled)
kartikaysaxena
force-pushed
the
schema-watch-mysql
branch
3 times, most recently
from
e5d02fd to
5031de0
Compare
internal/datastore/mysql/watch.go
Outdated
| return fmt.Errorf("unable to parse changed namespace: %w", err) | ||
| } | ||
| loaded = def | ||
| objName = def.Name |
There was a problem hiding this comment.
don't need to set objName, since its defined on the SchemaDefinition
Signed-off-by: Kartikay <[email protected]>
kartikaysaxena
force-pushed
the
schema-watch-mysql
branch
from
5031de0 to
b465181
Compare
implements schema watch for mysql datastore