Better Debug instances and __repr__ methods.
#891
Conversation
paraseba
force-pushed
the
push-xzkplxxuwkrr
branch
from
88eee6a to
c2b8a4a
Compare
Python's `SnapshotInfo` now also includes the Manifests info.
paraseba
force-pushed
the
push-xzkplxxuwkrr
branch
from
c2b8a4a to
2a04921
Compare
| written_at: val.flushed_at, | ||
| message: val.message, | ||
| metadata: val.metadata.into(), | ||
| manifests: val.manifests.into_iter().map(|v| v.into()).collect(), |
There was a problem hiding this comment.
Isn't this potentially a really big list? that will now be listed our with ancestry?
There was a problem hiding this comment.
Maybe we should have SnapshotInfo.list_manifests() instead?
There was a problem hiding this comment.
yes, but, ancestry is very slow anyway, since it fetches each snapshot from object store. Gathering the few manifests cannot add much
There was a problem hiding this comment.
Do you think the list of manifests could get very long with split? Don't forget this is only done when you try to print a snapshot, not something you would do a lot, or would expect to be fast.
There was a problem hiding this comment.
scratch, my latest about "print". What I should have said is: these objects get populated only by ancestry, which is very slow, and they are mostly used and discarded, I don't imagine gathering a few hundred extra objects will become a problem
There was a problem hiding this comment.
Ah I understand now. My goal with this was trying to give us a bit more insight to debug, using the lookup_snapshot method. I like the idea of having a dedicated ancestry representation, that could be text in the console, html in a notebook, etc
There was a problem hiding this comment.
An updating weather forecast repo might have O(100) manifests in each snapshot, one per array, for example.
But we are not fetching those manifests, only surfacing the data that is already in the snapshot, maybe 30 bytes per manifest or so.
There was a problem hiding this comment.
But this definitely has an impact on the repr for snapshots. Do you want me to skip manifests there? we can do that
There was a problem hiding this comment.
Yes, I'm only concerned about the repr, because we use it for ancestry right now.
Yes, let's just comment it out from the repr for now.
This will help for my update to test_can_read_old.py
Python's
SnapshotInfonow also includes the Manifests info.