Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Snapshot name collisions #4485
Snapshot name collisions #4485
Changes from 11 commits
d1f1021
7b7ac94
102d7ab
27d323b
4073b37
7e3894c
32fe99c
287a7ad
d211939
7225397
597decc
6d01bdf
5a5d627
91459b8
c93b59b
5709d60
9875ce5
06323cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to more strongly type the other layers as well (and actually wrote out the types as seen in the first commit), but it requires more work across the boundaries with not much gain right now. Will look more into it after fixing SummarizerNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think id (at that level) should be always null., no?
Based on latest discussion with SPO, I think we should start asserting in our layers that we provide either value (content but id === null) or reference (id !== null, but no other fields are provided) for trees, similar how we do for blobs - we either reuse existing one, or write out new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that storage always returns ID + all tree contents (i.e. this is the full skeleton). I think it could make sense for them to sometimes return partial trees, but I don't know exactly how that would be specified right now.
If there's a better way to type it that you know of, I can change it, but I know just from debugging that currently it will be id !== null and other fields are provided so that type would be misleading or at least limiting (maybe intentionally?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I though this interface is for writing snapshots (summaries), not for reading.
Runtime never reads "snapshots", it operates in individual (shallow) trees that always have content and ID.
BTW, we should be try to eliminate "snapshot" word from our code base. It is used as in "snapshot tree but I think naming is from the past. I think they are always shallow trees (i.e. only contain one level of data), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, should it be reverse? I.e. if we have it in snapshot, why do we read it from storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be an unrelated change I think, but it makes sense to me. I don't know as much about what it means when the blobs are stored directly in the snapshot though, maybe we prioritize the ones in storage for some reason? i.e. out of date or something?
@jatgarg would know I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe put some comment in here describing what legacy and next mean in practice?
Should be given them more descriptive names? While this is only runtime data, I image that we might have "next next" format some day :)