Storage: Add support for sortBy selector #80680
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
DanCech
changed the title
|
https://clusterpedia.io/docs/usage/search/multi-cluster/#sorting-by-multiple-fields -- is an interesting alternative. In that model, we would support something like: I like they this more explicitly flips things to a "search" context +1 for offset -- i can't think of a better option, the downside is if things get modified or deleted between page requests 😬 |
|
I would be absolutely fine with switching to label selectors which would give us more flexibility (and more concise kubectl cli syntax), if we do that would you want to keep folder as a field selector or just flip everything back to label selectors? |
DanCech
requested review from
papagian,
suntala and
idafurjes
and removed request for
a team
|
tests are now working, but the sqlite "database table is locked" issue is popping up when running migrations against sqlite. |
ryantxu
reviewed
Jan 23, 2024
|
yeah, I figured if the migrations had succeeded then the name didn't really
matter, but it won't hurt to increment it.
…On Tue, Jan 23, 2024 at 1:06 PM Ryan McKinley ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/services/store/entity/migrations/entity_store_mig.go
<#80680 (comment)>:
> @@ -120,7 +120,11 @@ func initEntityTables(mg *migrator.Migrator) string {
},
Indices: []*migrator.Index{
{Cols: []string{"guid", "resource_version"}, Type: migrator.UniqueIndex},
- {Cols: []string{"namespace", "group", "resource", "name", "resource_version"}, Type: migrator.UniqueIndex},
+ {
+ Cols: []string{"namespace", "group", "resource", "name", "resource_version"},
be sure the change the "marker" above so this migration happens on
existing setups. (yes it will wipe out everything first)
—
Reply to this email directly, view it on GitHub
<#80680 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALAOEY7XVDT6LDTVZIE4OTYP733HAVCNFSM6AAAAABB5SQXJ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZZGUZDGMJZGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
suntala
reviewed
Jan 29, 2024
pkg/services/grafana-apiserver/storage/entity/fieldRequirements.go
Outdated
Show resolved
Hide resolved
pkg/services/grafana-apiserver/storage/entity/fieldRequirements.go
Outdated
Show resolved
Hide resolved
pkg/services/grafana-apiserver/storage/entity/fieldRequirements.go
Outdated
Show resolved
Hide resolved
DanCech
force-pushed
the
unified-storage-sort-by
branch
from
f458665
to
3eb61fb
Compare
DanCech
changed the title
DanCech
changed the title
suntala
reviewed
Jan 30, 2024
DanCech
force-pushed
the
unified-storage-sort-by
branch
from
3eb61fb
to
902fee1
Compare
DanCech
force-pushed
the
unified-storage-sort-by
branch
from
bb19f34
to
7636595
Compare
Ukochka
pushed a commit
that referenced
this pull request
Feb 14, 2024
* add support for sortBy field selector * use label selectors instead of field selectors * set entity_labels on create & update * make entity server integration tests work * test fixes * be more consistent with handling of empty body, meta or status * workaround for database is locked errors during migration * fix double import of sqlite3 * rename functions and tidy up * refactor update * disable integration tests until we can fix the database locking issue
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 adds support for sorting lists by specifying a field selector like
grafana.app/sortBy="title", including support for paging.I opted to implement paging via offset rather than uids because of the complexity involved in building where clauses when sorting by multiple dimensions (e.g. for
grafana.app/sortBy=a;b desctheWHEREclause would need to be(a > ? OR (a = ? AND (b < ? OR (b = ? AND uid > ?)))) vs the simplicity of an offset-based approach.The other change from our standard semantics is that when specifying multiple columns the separator is a semicolon rather than a comma, using a comma resulted in an error from the k8s field selector parser like:
This appears to be due to the chained selectors syntax which supports sending multiple field selector expressions separated with commas.
Will take this out of draft status once I've had a chance to add tests.