@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.
Lint Skipped |
Unit Tests Skipped |
remotefilelog/basestore.py | ||
---|---|---|
45 | 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. | |
46 | If we're just counting, we can use _listkeys() instead, so we don't have to resolve the filenames. | |
54 | 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. | |
58 | 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 | ||
360 | I'd write out the full name debugpackstatus. st isn't a super common acronym in the code base. |
Ready for another look. Please check my C.
remotefilelog/basestore.py | ||
---|---|---|
58 | From our IRL chat, added verbose=False. |
Needs a test
remotefilelog/basestore.py | ||
---|---|---|
57–58 | os.path.join(self._getrootpath(), fnhashhex[:2], fnhashhex[2:]) would be the more platform agnostic way of doing this. |
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 | ||
---|---|---|
201 | 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. |
remotefilelog/debugcommands.py | ||
---|---|---|
376 | It's based on Ruby/Underscore.js's name for the same function. Then again, who ever said Ruby was a paragon of naming? |
remotefilelog/debugcommands.py | ||
---|---|---|
376 | 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? | |
384–386 | 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. |
remotefilelog/debugcommands.py | ||
---|---|---|
376 | 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). | |
384–386 | Interesting, I didn't know you could chain them. I'll take the shorter way. |
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.