-
Notifications
You must be signed in to change notification settings - Fork 9
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
database: change primary keys to use UUID's instead of integers #25
base: master
Are you sure you want to change the base?
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
98b6974
to
f9eec7f
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.
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.
@@ -415,5 +416,9 @@ func New(dataDir, dbName, username, host string) (*cockroachdb, error) { | |||
&InvoicePayment{}, | |||
) | |||
|
|||
// Add foreign key constraints |
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.
They're really indexes, not foreign key constraints, right?
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.
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.
@@ -61,7 +69,8 @@ func (i Identity) TableName() string { | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the User
field needed here?
string(stderrBytes)) | ||
} | ||
}() | ||
stderrBytes, errStderr := ioutil.ReadAll(stderr) |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.