Skip to content
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

Use Seek() and Read() APIs for index/data files instead of mmap + share expensive seeker resources among seekers #1421

Merged
merged 69 commits into from
Mar 26, 2019

Conversation

richardartoul
Copy link
Contributor

Had to create a new P.R for C.I reasons, see previous comments / discussion on #1299


var errorUnableToDetermineNumFieldsToSkip = errors.New("unable to determine num fields to skip")
errorUnableToDetermineNumFieldsToSkip = errors.New("unable to determine num fields to skip")
errorCalledDecodeBytesWithoutByteStreamDecoder = errors.New("called decodeBytes with out byte stream decoder")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename this to errThing... = rather than errorThing... = for consistency with other packages/etc.

// Should never happen, either something is really wrong with the code or
// the file on disk was corrupted
for {
currOffset, err := s.indexFd.Seek(0, os.SEEK_CUR)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this call necessary? Seems this is not needed because Seek(0, SEEK_CUR) should cause no change as SEEK_CUR means "seek relative to the current offset",

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just keep track of currOffset ourselves and avoid this syscall?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have looked at the different interfaces, looks difficult to actually call like .xmsgpackDecoder.Position() or whatever which can call the reader underneath.

Maybe just add a comment here that in the future we can remove this Seek (and consequently syscall) call by tracking the position ourselves with some refactoring of interfaces/etc?

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but perhaps we add the comment about removing the Seek(0, SEEK_CUR) syscall eventually?

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #1421 into master will increase coverage by 1.5%.
The diff coverage is 81.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1421     +/-   ##
========================================
+ Coverage    69.3%   70.9%   +1.5%     
========================================
  Files         838     842      +4     
  Lines       71823   72006    +183     
========================================
+ Hits        49838   51083   +1245     
+ Misses      18687   17572   -1115     
- Partials     3298    3351     +53
Flag Coverage Δ
#aggregator 82.3% <ø> (-0.1%) ⬇️
#cluster 85.8% <ø> (+11.1%) ⬆️
#collector 63.7% <ø> (ø) ⬆️
#dbnode 80.8% <81.5%> (-0.1%) ⬇️
#m3em 73.2% <ø> (+16.1%) ⬆️
#m3ninx 74.3% <ø> (+10.6%) ⬆️
#m3nsch 51.1% <ø> (-26.9%) ⬇️
#metrics 17.5% <ø> (ø) ⬆️
#msg 75% <ø> (+0.1%) ⬆️
#query 66% <ø> (ø) ⬆️
#x 76.7% <ø> (+1.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9205e8...273c035. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants