This is an archive of the discontinued Mercurial Phabricator instance.

packfiles: add hg debugpackstatus
AcceptedPublic

Authored by phillco on Nov 3 2017, 3:16 PM.

Details

Reviewers
singhsrb
durham
Group Reviewers
Restricted Project
Summary

@akushner thought it'd be helpful to have a dedicated debug command to run if packfile unpleasantness is suspected, and I thought it'd be useful too.

Also added some missing metrics on remotefilelog stores.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Nov 3 2017, 3:16 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 3 2017, 3:16 PM
phillco edited the summary of this revision. (Show Details)Nov 3 2017, 3:18 PM
phillco added a reviewer: singhsrb.
phillco added a subscriber: akushner.
phillco updated this revision to Diff 3289.Nov 5 2017, 9:25 PM
durham requested changes to this revision.Nov 6 2017, 2:18 PM
durham added a subscriber: durham.
durham added inline comments.
remotefilelog/basestore.py
46

We may want a try/catch around all of these metric functions that call _getfiles. _getfiles could throw an exception if the directory isn't readable for some reason, and we don't want metric logging code to bring down the process. Or we add the try/catch to _getfiles or _listkeys and print a warning if the directory is inaccesible, then return an empty list.

47

If we're just counting, we can use _listkeys() instead, so we don't have to resolve the filenames.

55

If we're just getting the file size, we can use _listkeys to get the information necessary to compute the file path to get the size.

59

Isn't getmetrics called at store creation time now? By your previous diff. If so, getnumfiles is expensive right? How do we ensure that we don't accidentally call getnumfiles every time we want to log metrics.

remotefilelog/debugcommands.py
361

I'd write out the full name debugpackstatus. st isn't a super common acronym in the code base.

This revision now requires changes to proceed.Nov 6 2017, 2:18 PM
phillco marked 3 inline comments as done.Nov 8 2017, 12:33 AM
phillco edited the summary of this revision. (Show Details)
phillco retitled this revision from remotefilelog: add hg debugpackstatus to packfiles: add hg debugpackstatus.
phillco updated this revision to Diff 3331.
phillco planned changes to this revision.Nov 8 2017, 12:33 AM

.

phillco updated this revision to Diff 3506.Nov 14 2017, 10:05 PM
phillco marked an inline comment as done.Nov 14 2017, 10:07 PM
phillco updated this revision to Diff 3507.
phillco marked an inline comment as done.Nov 14 2017, 10:23 PM
phillco updated this revision to Diff 3509.

Ready for another look. Please check my C.

remotefilelog/basestore.py
59

From our IRL chat, added verbose=False.

durham requested changes to this revision.Nov 15 2017, 6:56 PM

Needs a test

remotefilelog/basestore.py
58–59

os.path.join(self._getrootpath(), fnhashhex[:2], fnhashhex[2:]) would be the more platform agnostic way of doing this.

This revision now requires changes to proceed.Nov 15 2017, 6:56 PM
phillco marked an inline comment as done.Nov 16 2017, 1:32 AM
phillco updated this revision to Diff 3564.Nov 16 2017, 1:32 AM
phillco planned changes to this revision.Nov 16 2017, 1:35 AM
phillco updated this revision to Diff 3886.Nov 27 2017, 3:00 PM
phillco updated this revision to Diff 3887.Nov 27 2017, 3:03 PM

I flushed this out a bit since the last change, adding getmetrics() to datapackstore and printing the paths for remotefilelog stores, too.

remotefilelog/debugcommands.py
202

Not really related, but I noticed that debugdatapack aborted too early if one of the paths had any errors. I could split this out if you think it's warranted.

durham accepted this revision.Nov 28 2017, 7:32 PM
durham added inline comments.
remotefilelog/debugcommands.py
377

I'd rename this 'removenone' or something.

416

I'd use the words "File Data Store" and "File History Store" in the user facing code. content and metadata were old terms and should probably be replaced throughout the code.

This revision is now accepted and ready to land.Nov 28 2017, 7:32 PM
phillco added inline comments.Nov 28 2017, 7:53 PM
remotefilelog/debugcommands.py
377

It's based on Ruby/Underscore.js's name for the same function. Then again, who ever said Ruby was a paragon of naming?

quark added a subscriber: quark.Nov 28 2017, 8:02 PM
quark added inline comments.
remotefilelog/basestore.py
47

Maybe just count = len(list(self._listkeys()) since that's shorter and does not need placeholders

remotefilelog/debugcommands.py
202

No need to split.

377

Could you use filter(None, list)? It is more common in python.

phillco added inline comments.Nov 28 2017, 9:18 PM
remotefilelog/basestore.py
47

clever, thx

remotefilelog/debugcommands.py
377

@quark looks like that removes other falsy values too, like 0. But, I guess that is desirable?

quark added inline comments.Nov 28 2017, 10:51 PM
remotefilelog/debugcommands.py
377

Yes. Whether 0 or [] should be False is a separate topic (I personally prefer the Ruby/Lua way - only nil and false are False). It seems you're using it to filter paths, which is fine I guess?

385–387

Maybe more common in Python:

[p for s in store.stores if s for p in getpaths(s)]

It's much shorter. Although I personally do not enjoy list comprehensive that much.

phillco added inline comments.Nov 29 2017, 1:29 AM
remotefilelog/debugcommands.py
377

Yeah, it doesn't matter today but I was looking ahead if we were to generalize this function / move it into a util. Then I'd expect it to behave the Ruby way too (or at least have a different name, like filterfalsy).

385–387

Interesting, I didn't know you could chain them. I'll take the shorter way.