-
Notifications
You must be signed in to change notification settings - Fork 453
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
[dbnode/index] postingsList caching for FieldQuery #1530
Conversation
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
Weird about the code cov dropping so much? Probably a bug with the reporting I suppose? |
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.
Nice!
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
case PatternTypeField: | ||
q.metrics.field.puts.Inc(1) | ||
default: | ||
q.metrics.unknown.puts.Inc(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.
super nit: Can you add a "should never happen" under the default case so it doesnt seem like thats something that might happen regularly
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.
sure thing
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
return s.reader.MatchField(field) | ||
} | ||
|
||
// TODO(rartoul): Would be nice to not allocate strings here. |
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 making me look even lazier :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.
;)
fe30a69
to
f96935f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1530 +/- ##
======================================
Coverage 70.2% 70.2%
======================================
Files 851 851
Lines 73030 73030
======================================
Hits 51271 51271
Misses 18325 18325
Partials 3434 3434
Continue to review full report at Codecov.
|
No description provided.