-
Notifications
You must be signed in to change notification settings - Fork 8
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
Import axon as an additional internal project, support JSONB fields in Changeset and expand demo #9
Conversation
Axon is a separate project which should be integrated into Warp Pipe
JSON/B fields are stored in the NewValue ChangesetColumn as a map[string]interface {} which the "sql" package does not support. Solution: Attempt to convert the map to a string or Fatal.
dc71281
to
ff9ed0f
Compare
} | ||
log.Printf("row inserted: %s", change) |
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.
These may be too noisy. Perhaps should be a roll up reported every 10-30 seconds? See #1
Resolves error from sql package: unsupported type []interface {}, a slice of interface.
Resolves: could not find name id in map[string]interface {}
01f9c73
to
781fc4c
Compare
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.
This looks pretty good to me, plus it has already been proven. Just some minor comments.
// TODO: More thorough version checks | ||
major, err := strconv.Atoi(version[0]) | ||
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.
I think these could be panics, or have some context added to the messages.
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.
Absolutely. There's a lot of UX to add. I'll add this as a todo.
|
||
for _, r := range rows { | ||
colDefault := strings.Split(r.ColumnDefault, "'") | ||
sequenceName := colDefault[1] |
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.
Im guessing it is safe not to check the length?
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.
There's a lot of additional error checking needed in this entire codebase, but, yes, unless there is a database is broken this should never fail.
Sequence values are not updated when a row is inserted including the sequenced value, often the ID. Checks for sequenced columns on startup and then runs setval() after each insert to manually update the value for that column.
InsertRow needs to load data from the sourceDB in the next commit
There is no way to detect sequence updates directly, so we'll assume they are changed on INSERT and update the orphaned sequences.
Handles: unsupported type []interface {}, a slice of interface when the array is not empty
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
cmd/axon/schema.go
Outdated
var primaryKeys = make(map[string][]string) | ||
|
||
// maps serial key columns by table | ||
var columnSequences = make(map[string]string) |
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.
nit: perhaps rename this since it read as if its the order of the columns in a table, and not the colums that are of a sequence type. maybe something like sequenceColumns
instead
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.
Swapped variable name
err := conn.Select(&rows, ` | ||
SELECT | ||
t.table_name, | ||
string_to_array(string_agg(c.column_name, ','), ',') AS primary_key |
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's been a while since I wrote this, but iirc this query is slightly different for other versions of PG (older, I believe). Just making note because i can't remember if its the meta-data tables or the method for going making the array that I had to modify at some point.
return "", false | ||
} | ||
|
||
func updateColumnSequence(conn *sqlx.DB, table string, columns []*warppipe.ChangesetColumn) error { |
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.
Please pluralize the name since it takes multiple columns s/updateColumnSequence/updateColumnSequences/g
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.
done
cmd/axon/schema.go
Outdated
return nil | ||
} | ||
|
||
// loadUnconnectedSequences loads all sequences in the source database not |
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.
Docstring and function name do not match. Should be loadOrphanSequences
.
cmd/axon/schema.go
Outdated
for _, sequenceName := range orphanSequences { | ||
var lastVal int64 // PG bigint is 8bit | ||
|
||
err := sourceDB.QueryRow(` |
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.
nit: you could also do
err := sourceDB.Get(&lastVal, "SELECT last_value FROM " + sequenceName)
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.
unless the target pointer needs to be a struct, in which case nvm what I said :P
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.
Yeah your version is cleaner, switching
cmd/axon/schema.go
Outdated
} | ||
|
||
var setVal string | ||
err = targetDB.QueryRow(` |
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.
ditto here for all the other QueryRow
calls
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.
done
cmd/axon/sql.go
Outdated
warppipe "github.com/perangel/warp-pipe" | ||
) | ||
|
||
var regexOneSpace = regexp.MustCompile(`\s+`) |
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.
nit: s/regexOneSpace/regexSpace/g
since this regex is technically 1 or more spaces
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.
done
db/schema.go
Outdated
log.Printf("%v+", pgErr) | ||
} | ||
// Failure case resolution: | ||
// GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA public TO master; |
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.
This is RDS specific, please remove this.
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.
removed
Import axon as an additional internal project, support JSONB fields in Changeset and expand demo
fixups have been rebased out. Merge commit changed to 02f4352 |
Demo run: