Storage: Support listing deleted entities #84043
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
toddtreece
reviewed
Mar 7, 2024
| entityQuery.offset = continueToken.StartOffset | ||
| } | ||
|
|
||
| if len(r.Labels) > 0 { |
There was a problem hiding this comment.
i'm trying to parse the differences between the two list functions. it looks like this if block is one of them. are the other two major differences the table entity_history and entityQuery.addWhere("action", entity.Entity_DELETED)?
toddtreece
approved these changes
Mar 7, 2024
There was a problem hiding this comment.
currently it seems like there's additional duplication, but it's kind of a coin toss to know which approach is the best long term. this can always be refactored if the duplication becomes an issue, but leaving that up to @DanCech's expertise
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR explores adding support for listing deleted entities by passing a new label selector
grafana.app/listDeleted=trueIt works by selecting the entries that are added to the entity_history_table when entities are deleted.
I opted to separate it from the regular List function as there are some differences around label selection (we can't use the entity_labels table and instead use string matching against the labels column), but we could fold them together and just split out that section.
If we do keep them separate it may make sense to promote ListDeleted to a separate grpc method which would avoid the need for the Deleted flag in EntityListRequest
I did look into adding support for proper json functions into the dialects, but managing the different syntaxes and behaviors across postgres, mysql and sqlite was quite complex and brittle.