Page MenuHomePhabricator

remotetreedatastore: make MissingNodesError subclass of KeyError

Authored by singhsrb on Dec 15 2017, 4:11 PM.


Group Reviewers
Restricted Project

The union content store

  • iterates through all the stores it has until the current store has the content.
  • Or, it fails eventually if none of the stores have the content.

It does so by relying on the current store throwing a KeyError if it doesn't
have the content.

remotetreedatastore was throwing the MissingNodesError which means any
remaining stores after it would not even get a chance to look for the content.
This commit addresses the same.

Test Plan

Ran all the tests.

Diff Detail

rFBHGX Facebook Mercurial Extensions
Lint Skipped
Unit Tests Skipped

Event Timeline

singhsrb created this revision.Dec 15 2017, 4:11 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 15 2017, 4:11 PM
quark added a subscriber: quark.Dec 15 2017, 7:22 PM
quark added inline comments.

Maybe just make this a KeyError:

class MissingNodesError(error.Abort, KeyError):

That's what error.LookupError does for revlogs.

singhsrb added inline comments.Dec 15 2017, 7:52 PM


singhsrb edited the summary of this revision. (Show Details)Dec 15 2017, 7:58 PM
singhsrb retitled this revision from remotetreedatastore: throw KeyError instead of MissingNodesError to remotetreedatastore: make MissingNodesError subclass of KeyError.
singhsrb updated this revision to Diff 4524.
quark accepted this revision.Dec 15 2017, 8:03 PM

Maybe the test also needs to be updated.

This revision is now accepted and ready to land.Dec 15 2017, 8:03 PM

the test test-treemanifest-infinitepush.t (included in this commit) reflects the change in behavior because of the change in error hierarchy. Am I missing something?

I see. I misunderstood how this works. The KeyError is a different exception generated in a different place.

singhsrb abandoned this revision.Jan 31 2018, 5:45 PM

Will be rebasing and floating for review again.