-
Notifications
You must be signed in to change notification settings - Fork 455
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
Implement the ability to write in batches in M3DB database #1157
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1157 +/- ##
========================================
- Coverage 71.5% 71.5% -0.1%
========================================
Files 726 728 +2
Lines 60839 61085 +246
========================================
+ Hits 43540 43683 +143
- Misses 14523 14602 +79
- Partials 2776 2800 +24
Continue to review full report at Codecov.
|
@@ -45,17 +45,25 @@ var ( | |||
timeZero = time.Time{} | |||
) | |||
|
|||
// WritesBatch is a batch of Write. | |||
type WritesBatch []Write |
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.
Can we wrap this up in a struct (which we could then later pool)?
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.
Can we also rename it to WriteBatch
and drop the s
? Just since in other places where we have write batches that's what we call them.
Tags ident.Tags // FOLLOWUP(prateek): wire Tags to commit log writer | ||
Tags ident.Tags | ||
|
||
TagIter ident.TagIterator |
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.
Just curious, why do we have both now? Is it an either/or argument?
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.
Its an either/or kind of thing. It kind of sucks but you have to turn this into a partially filled struct thing so you don't have ot convert one massive slice of one thing into another massive slice of another thing. I'll do some cleanup, add some comments, and try to see if i can explain / document this in a sane manner
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.
Can we create a wrapper type TagsOrTagsIter
struct and perhaps a type TagsType uint
enum like to specify which one to use?
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 for now
0190262
to
b647dc0
Compare
No description provided.