Conversation
|
@DeanPDX My turn to tell you to check this out... It's not entirely ready for merge (I still want to write some additional tests and figure out a few other things first) but I wouldn't mind a quick look-through. I tried to keep changes atomic, which each commit handling one specific method/set of methods. |
toptobes
force-pushed
the
KG-collection-updates
branch
from
04fe1af to
525144c
Compare
toptobes
force-pushed
the
KG-collection-updates
branch
from
525144c to
842fbcb
Compare
DeanPDX
left a comment
DeanPDX
left a comment
There was a problem hiding this comment.
Lookin' good so far! I have some comments. I think biggest thing we need to think about is that errors.Is thing and concurrency questions.
| var doc testDoc | ||
| if err := c.Decode(&doc); err != nil { | ||
| t.Fatalf("Decode failed: %v", err) | ||
| t.Fatalf("DecodeID failed: %v", err) |
There was a problem hiding this comment.
Why do these say DecodeID when they are calling Decode?
| @@ -0,0 +1,195 @@ | |||
| // Copyright IBM Corp. | |||
There was a problem hiding this comment.
Not sure about the file name here. I would contend that "utils" and "helpers" is almost always not idiomatic (though I'm guilty of this with "testutils" package; in that case I couldn't think of a better package name and gave up).
|
|
||
| // DecodeID unmarshalls the inserted ID into v. | ||
| // v should be a pointer to the appropriate ID type (string, int, ObjectId, UUID, etc.) | ||
| func (r *InsertOneResult) DecodeID(v any) error { |
There was a problem hiding this comment.
When we get generic method receivers we can make this a little easier to consume.
| // for cursor.Next(ctx) { | ||
| // var doc MyDocument | ||
| // if err := cursor.Decode(&doc); err != nil { | ||
| // if err := cursor.DecodeID(&doc); err != nil { |
There was a problem hiding this comment.
I think DecodeID might have been globally replaced too much with find/replace or something.
| var allWarnings results.Warnings | ||
| errChan := make(chan error, 1) | ||
|
|
||
| for w := 0; w < *opts.Concurrency; w++ { |
There was a problem hiding this comment.
I may have missed it, but, should we guard against concurrency values <= 0?
| slice := records.Slice(start, end).Interface() | ||
| res, warn, err := runInsertMany(ctx, slice, mkCmd, opts) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
I'm not 100% sure about this. If we encounter an error, the "first" error wins. But the other goroutines keep spinning away. And that buffer is full after 1 error so the first error wins, right? Here's a go playground link that tests/illustrates this:
https://go.dev/play/p/2gpA76ye97T
(I always have to test my concurrency stuff because it can get complicated quickly)
Is that the desired behavior? It seems like you are trying to differentiate between a critical error (err) and api errors that we batch up and send back. If we hit a critical error, do you think we should cancel the other goroutines? I'm honestly not sure but wanted to just at least think about this with you out loud. :)
| }, opts.APIOptions) | ||
|
|
||
| b, warnings, execErr := cmd.Execute(ctx) | ||
| if execErr != nil && !errors.Is(execErr, &DataAPIError{}) { |
There was a problem hiding this comment.
I think errors.Is is almost certainly not doing what you want it to here. I think you are wanting to type-assert that execErr is NOT a DataAPIError, right? Maybe something like this?
var apiErr *DataAPIError
if execErr != nil && !errors.As(execErr, &apiErr) {
return nil, warnings, execErr
}Docs: https://pkg.go.dev/errors#example-As
Also - there is a new AsType but I think it's so recent we can't use it yet:
| }, nil | ||
| } | ||
|
|
||
| // collectionFindOneAndUpdatePayload is the payload for the findOneAndUpdate command. |
There was a problem hiding this comment.
Oh one more thing. Copy/paste error here. A tale as old as time.
| return result, nil | ||
| } | ||
|
|
||
| // collectionFindOneAndUpdatePayload is the payload for the findOneAndUpdate command. |
There was a problem hiding this comment.
One other copy/paste error.
| Projection map[string]any `json:"projection,omitempty"` | ||
| } | ||
|
|
||
| // FindOneAndUpdate finds a single document matching the filter, applies the update, |
There was a problem hiding this comment.
One LAST copy/paste thing. Probably should be something along the lines of:
// FindOneAndDelete finds a single document matching the filter and deletes it...I need to check the docs though to see if there are any special circumstances to think about here. But I bet you know exactly what it does off the top of your head so I'm not unless you want me to! 🤷
2 participants
No description provided.