database: change primary keys to use UUID's instead of integers#25
database: change primary keys to use UUID's instead of integers#25orthomind wants to merge 1 commit intodecred:masterfrom
Conversation
Change primary keys to use UUID's as recommended by CockroachDB's documentation. Change includes: - Update primary key's to UUID's - Fix bug with dataloader that would sometimes cause it to fail - Change some output from fmt to log to allow using logging options
orthomind
force-pushed
the
feature/use_uuid_for_pk
branch
from
98b6974 to
f9eec7f
Compare
sndurkin
left a comment
sndurkin
left a comment
There was a problem hiding this comment.
Thanks for your PR! And thanks for switching fmt -> log, I agree that is better. I had a few comments which I've added inline.
| &InvoicePayment{}, | ||
| ) | ||
|
|
||
| // Add foreign key constraints |
There was a problem hiding this comment.
They're really indexes, not foreign key constraints, right?
There was a problem hiding this comment.
Yep, I had some foreign key stuff in there too (which the indexes were required for), but I removed the foreign key stuff as I was having a warning and thought I'd bundled enough in this PR already. I will update the comment.
| type Invoice struct { | ||
| Token string `gorm:"primary_key"` | ||
| UserID uint `gorm:"not_null"` | ||
| User User `gorm:"foreignkey:UserID;association_foreignkey:ID"` |
There was a problem hiding this comment.
Is the User field needed here?
| gorm.Model | ||
| UserID uint `gorm:"not_null"` | ||
| Model | ||
| User User `gorm:"foreignkey:UserID"` |
There was a problem hiding this comment.
Is the User field needed here?
| string(stderrBytes)) | ||
| } | ||
| }() | ||
| stderrBytes, errStderr := ioutil.ReadAll(stderr) |
There was a problem hiding this comment.
I think this change may be causing an issue with the dataload. If you run it with tests, like this:
cmswwwdataload -v -includetests
it gets stuck (I think) when it calls testInventory. This is the last of the output I see, and the program doesn't seem to terminate:
2019/02/07 23:00:46 Fetching all invoices
2019/02/07 23:00:46 $ cmswwwcli --host https://127.0.0.1:4443 --jsonout invoices
There was a problem hiding this comment.
Strange. I've tried this again a handful of times using cmswwwdataload --verbose --deletedata --includetests, but haven't seen it get stuck.
You're trying this on mac, right? I might have to see if I can get my hands on one to test with. Just to eliminate a couple of things:
- Did you update the vendor folder with
GO111MODULE=on go mod vendor? - What's your Go version?
There was a problem hiding this comment.
Actually, I'm not even seeing that line in my output ('Fetching all invoices'). When I try to run it manually though, it works fine. Do you know why I might not be seeing that particular test?
There was a problem hiding this comment.
@sndurkin Are you able to provide me with a little more information to help me track down what's going wrong here please? Have some questions above for you in this conversation thread.
There was a problem hiding this comment.
Hey @orthomind, sorry for the lack of response, been really busy lately.
You're trying this on mac, right?
I'm on Windows.
Did you update the vendor folder with
GO111MODULE=on go mod vendor
Yeah, I did GO111MODULE=on go mod vendor && go install ./....
What's your Go version?
go version go1.11.4 windows/amd64
Actually, I'm not even seeing that line in my output ('Fetching all invoices').
I'm not really sure why you're not seeing that output. If you look at cmswwwdataload/main.go, you can see that if cfg.IncludeTests is true, it calls testInventory, which calls GetAllInvoices, which prints the 'Fetching all invoices' log.
Here's the full output that I'm seeing: https://gist.github.com/sndurkin/86b71cf5787b0fad988d1169a4b304f0
2 participants
Change primary keys to use UUID's as recommended by CockroachDB's
documentation. Change includes:
This change will require re-vendoring ('task vendor' or 'GO111MODULE=on go mod vendor').
This PR took me longer than expected because it was difficult to track down the irregular dataloader failure. As a result of this, I included conversions from fmt to log for some output so that the logger can optionally be configured to include file and line numbers.