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

Fix uninitialized fields in file metadata #4693

Closed
wants to merge 1 commit into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Nov 16, 2018

This is a quick fix for the uninitialized bugs in LiveFileMetaData and SstFileMetaData that were uncovered in #4686.

Test Plan:

  • make check -j64
  • manual testing for read counts since the behavior is non-deterministic
In [23]: lookup_key = next(filter(lambda file: file['name'] == '/000973.sst', db.get_live_files_metadata()))['smallestkey']

In [24]: for i in range(100000):
    ...:     db.get(lookup_key)
    ...:

In [25]: next(filter(lambda file: file['name'] == '/000973.sst', db.get_live_files_metadata()))['num_reads_sampled']
Out[25]: 109568

@ajkr
Copy link
Contributor Author

ajkr commented Nov 16, 2018

#4686 attempts to create a long-term fix and will take a bit longer to figure out. I'd prefer to land this hopefully uncontroversial PR in the meantime.

Copy link
Contributor

@abhimadan abhimadan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching this

@siying
Copy link
Contributor

siying commented Nov 16, 2018

Thank you for fixing it. Too bad I didn't realize this obvious bug when I introduced the counter.

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

LGTM. I hope we can address this problem more thoroughly in your separate PR #4686

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

None yet

5 participants