-
Notifications
You must be signed in to change notification settings - Fork 691
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
Correcting bytes read value with respect to metadata loading #1769
Conversation
- because there is a method receiving struct by value rather than reference. this causes a read/write problem just because pass by value causes a read on the shared variable when there are writes elsewhere.
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.
Jon should be able to get numbers from your toy build by Thursday.
index/scorch/snapshot_segment.go
Outdated
// segment was mmaped recently, in which case | ||
// we consider the loading cost of the metadata | ||
// as part of IO stats. | ||
mmaped uint64 |
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.
Move this to be the first attribute of this struct. Also, a uint32 would suffice?
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 it's sufficient, will do.
index/scorch/merge.go
Outdated
@@ -429,6 +430,7 @@ type segmentMerge struct { | |||
oldNewDocNums map[uint64][]uint64 | |||
new segment.Segment | |||
notifyCh chan *mergeTaskIntroStatus | |||
mmaped bool |
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'd rather we keep the type of this variable as uint for consistency.
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.
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.
Tentatively approving this.
Let's wait on the results before merging.
For fix: blevesearch/bleve#1769 Change-Id: I57d39397a96412f770073c0653d1e68076500b70 Reviewed-on: https://review.couchbase.org/c/cbft/+/184147 Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Sitaram Vemulapalli <sitaram.vemulapalli@couchbase.com> Tested-by: Abhi Dangeti <abhinav@couchbase.com>
No description provided.