Copilot commented on code in PR #1422:
URL: https://github.com/apache/dubbo-admin/pull/1422#discussion_r2901555306
##########
pkg/store/memory/store.go:
##########
@@ -63,14 +74,42 @@ func (rs *resourceStore) Start(_ runtime.Runtime, _ <-chan
struct{}) error {
}
func (rs *resourceStore) Add(obj interface{}) error {
- return rs.storeProxy.Add(obj)
+ if err := rs.storeProxy.Add(obj); err != nil {
+ return err
+ }
+ r, ok := obj.(coremodel.Resource)
+ if ok {
+ rs.addToTrees(r)
+ }
+ return nil
}
func (rs *resourceStore) Update(obj interface{}) error {
- return rs.storeProxy.Update(obj)
+ r, ok := obj.(coremodel.Resource)
+ if ok {
+ // Get the old resource from the store to properly remove it
from trees
+ oldObj, exists, err := rs.storeProxy.Get(r)
+ if exists && err == nil {
+ if oldRes, ok := oldObj.(coremodel.Resource); ok {
+ rs.removeFromTrees(oldRes)
+ }
+ }
+ }
+ if err := rs.storeProxy.Update(obj); err != nil {
+ return err
+ }
+ if ok {
+ // Add new entry with updated values
+ rs.addToTrees(r)
+ }
+ return nil
}
func (rs *resourceStore) Delete(obj interface{}) error {
+ r, ok := obj.(coremodel.Resource)
+ if ok {
+ rs.removeFromTrees(r)
Review Comment:
`Delete` removes entries from the prefix trees using the object passed in.
If callers delete using a minimal object (or a tombstone) that doesn't contain
the original indexed fields, `removeFromTrees` won't compute the same index
values and stale RadixTree entries will remain. To make deletes reliable, fetch
the existing stored resource by key (similar to `Update`) and remove based on
that stored object before deleting.
```suggestion
// Fetch the existing stored resource by key to reliably remove
from prefix trees.
if existingObj, exists, err :=
rs.storeProxy.GetByKey(r.ResourceKey()); err == nil && exists {
if existingRes, ok := existingObj.(coremodel.Resource);
ok {
rs.removeFromTrees(existingRes)
}
}
```
##########
pkg/store/memory/store.go:
##########
@@ -119,7 +176,20 @@ func (rs *resourceStore) GetIndexers() cache.Indexers {
}
func (rs *resourceStore) AddIndexers(newIndexers cache.Indexers) error {
- return rs.storeProxy.AddIndexers(newIndexers)
+ rs.treesMu.Lock()
+ defer rs.treesMu.Unlock()
+
+ if err := rs.storeProxy.AddIndexers(newIndexers); err != nil {
+ return err
+ }
+
+ // Add RadixTrees for new indexers
+ for indexName := range newIndexers {
+ if _, exists := rs.prefixTrees[indexName]; !exists {
+ rs.prefixTrees[indexName] = radix.New()
+ }
+ }
Review Comment:
`AddIndexers` creates new RadixTrees for newly added indexers, but it never
backfills existing stored resources into those new trees. After adding an
indexer to a non-empty store, HasPrefix queries on that index will return no
results until resources are re-added/updated. Consider iterating over existing
items (e.g., `rs.storeProxy.List()`) and populating the new tree(s) immediately.
```suggestion
}
// Backfill newly added radix trees with existing items in the store so
that
// prefix-based queries on the new indexes work immediately.
existingItems := rs.storeProxy.List()
for indexName, indexFunc := range newIndexers {
tree, exists := rs.prefixTrees[indexName]
if !exists {
continue
}
for _, obj := range existingItems {
indexedValues, err := indexFunc(obj)
if err != nil {
return err
}
for _, v := range indexedValues {
if v == "" {
continue
}
// We only need the key in the radix tree for
prefix lookups, so the value
// can be a trivial placeholder.
tree.Insert(v, struct{}{})
}
}
}
```
##########
pkg/store/memory/store.go:
##########
@@ -183,17 +253,29 @@ func (rs *resourceStore) PageListByIndexes(indexes
map[string]string, pq coremod
return pageData, nil
}
-func (rs *resourceStore) getKeysByIndexes(indexes map[string]string)
([]string, error) {
+func (rs *resourceStore) getKeysByIndexes(indexes []index.IndexCondition)
([]string, error) {
if len(indexes) == 0 {
return []string{}, nil
Review Comment:
When no index conditions are provided, `getKeysByIndexes` returns an empty
slice (so `ListByIndexes` returns no resources). The DB-backed store treats
empty conditions as "return all resources" (and there’s a test for that), so
this makes behavior inconsistent across store implementations. Consider
returning `rs.ListKeys()` here to align semantics.
```suggestion
// No index conditions means "no filtering": return all keys to
match DB-backed store semantics.
return rs.storeProxy.ListKeys(), nil
```
##########
pkg/store/dbcommon/gorm_store.go:
##########
@@ -555,12 +617,102 @@ func (gs *GormStore) clearIndices() {
gs.indices.Clear()
}
+// persistIndexEntries writes index entries for a resource to the database
+// If oldResource is not nil, first deletes old entries, then inserts new ones
+func (gs *GormStore) persistIndexEntries(resource model.Resource, oldResource
model.Resource) error {
+ db := gs.pool.GetDB()
+
+ // Delete old entries if updating
+ if oldResource != nil {
+ if err := db.Where("resource_key = ?",
oldResource.ResourceKey()).Delete(&ResourceIndexModel{}).Error; err != nil {
+ return err
+ }
+ }
+
+ // Get all index entries for this resource
+ indexers := gs.indices.GetIndexers()
+ var entries []ResourceIndexModel
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ for _, v := range values {
+ entries = append(entries, ResourceIndexModel{
+ ResourceKind: gs.kind.ToString(),
+ IndexName: indexName,
+ IndexValue: v,
+ ResourceKey: resource.ResourceKey(),
+ })
+ }
+ }
+
+ if len(entries) == 0 {
+ return nil
+ }
+
+ return db.Create(&entries).Error
+}
+
+// deleteIndexEntries removes all index entries for a resource key
+func (gs *GormStore) deleteIndexEntries(resourceKey string) error {
+ db := gs.pool.GetDB()
+ return db.Where("resource_key = ?",
resourceKey).Delete(&ResourceIndexModel{}).Error
+}
Review Comment:
`deleteIndexEntries` deletes by `resource_key` only. Because resource keys
are not unique across kinds, this can remove index entries for other resource
kinds. Filter by both `resource_kind` and `resource_key` to avoid cross-kind
data loss.
##########
pkg/store/dbcommon/gorm_store.go:
##########
@@ -555,12 +617,102 @@ func (gs *GormStore) clearIndices() {
gs.indices.Clear()
}
+// persistIndexEntries writes index entries for a resource to the database
+// If oldResource is not nil, first deletes old entries, then inserts new ones
+func (gs *GormStore) persistIndexEntries(resource model.Resource, oldResource
model.Resource) error {
+ db := gs.pool.GetDB()
+
+ // Delete old entries if updating
+ if oldResource != nil {
+ if err := db.Where("resource_key = ?",
oldResource.ResourceKey()).Delete(&ResourceIndexModel{}).Error; err != nil {
Review Comment:
When updating, old index entries are deleted with `WHERE resource_key = ?`
only. Since `ResourceKey()` is just `mesh/name` (not globally unique across
resource kinds), this can delete index rows belonging to other kinds that share
the same key. Include `resource_kind = ?` (and ideally also `index_name` if you
want narrower deletes) in the delete filter.
```suggestion
if err := db.Where("resource_key = ? AND resource_kind = ?",
oldResource.ResourceKey(),
gs.kind.ToString()).Delete(&ResourceIndexModel{}).Error; err != nil {
```
##########
pkg/store/dbcommon/gorm_store.go:
##########
@@ -555,12 +617,102 @@ func (gs *GormStore) clearIndices() {
gs.indices.Clear()
}
+// persistIndexEntries writes index entries for a resource to the database
+// If oldResource is not nil, first deletes old entries, then inserts new ones
+func (gs *GormStore) persistIndexEntries(resource model.Resource, oldResource
model.Resource) error {
+ db := gs.pool.GetDB()
+
+ // Delete old entries if updating
+ if oldResource != nil {
+ if err := db.Where("resource_key = ?",
oldResource.ResourceKey()).Delete(&ResourceIndexModel{}).Error; err != nil {
+ return err
+ }
+ }
+
+ // Get all index entries for this resource
+ indexers := gs.indices.GetIndexers()
+ var entries []ResourceIndexModel
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ for _, v := range values {
+ entries = append(entries, ResourceIndexModel{
+ ResourceKind: gs.kind.ToString(),
+ IndexName: indexName,
+ IndexValue: v,
+ ResourceKey: resource.ResourceKey(),
+ })
+ }
+ }
+
+ if len(entries) == 0 {
+ return nil
+ }
+
+ return db.Create(&entries).Error
+}
+
+// deleteIndexEntries removes all index entries for a resource key
+func (gs *GormStore) deleteIndexEntries(resourceKey string) error {
+ db := gs.pool.GetDB()
+ return db.Where("resource_key = ?",
resourceKey).Delete(&ResourceIndexModel{}).Error
+}
+
+// getKeysByPrefixFromDB retrieves resource keys matching a prefix from the
database
+func (gs *GormStore) getKeysByPrefixFromDB(indexName, prefix string)
([]string, error) {
+ db := gs.pool.GetDB()
+ var entries []ResourceIndexModel
+ err := db.Where("resource_kind = ? AND index_name = ? AND index_value
LIKE ?",
+ gs.kind.ToString(), indexName, prefix+"%").
+ Find(&entries).Error
Review Comment:
Prefix matching uses `index_value LIKE prefix + '%'`. If `prefix` can
contain SQL wildcard characters (`%` / `_`), the query will match more than a
literal prefix. If the intent is literal prefix semantics, escape wildcards in
`prefix` (and add an `ESCAPE` clause) before building the LIKE pattern.
##########
pkg/store/dbcommon/gorm_store.go:
##########
@@ -343,6 +370,32 @@ func (gs *GormStore) Replace(list []interface{}, _ string)
error {
gs.indices.UpdateResource(resource, nil)
}
+ // Persist all index entries in bulk
+ var indexEntries []ResourceIndexModel
+ indexers := gs.indices.GetIndexers()
+ for _, resource := range resources {
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ for _, v := range values {
+ indexEntries = append(indexEntries,
ResourceIndexModel{
+ ResourceKind:
gs.kind.ToString(),
+ IndexName: indexName,
+ IndexValue: v,
+ ResourceKey:
resource.ResourceKey(),
+ })
+ }
+ }
+ }
+
+ if len(indexEntries) > 0 {
+ if err := tx.CreateInBatches(&indexEntries, 100).Error;
err != nil {
+ logger.Warnf("failed to persist index entries
during replace: %v", err)
Review Comment:
During `Replace`, failure to persist `resource_indices` is only logged and
the transaction still commits. That can leave follower replicas without usable
persisted indexes until a full rebuild occurs. If persisted indexes are
required for correctness in multi-replica mode, consider returning the error so
the transaction rolls back (or at least make the behavior configurable).
```suggestion
logger.Warnf("failed to persist index entries
during replace: %v", err)
return err
```
##########
pkg/store/dbcommon/gorm_store.go:
##########
@@ -555,12 +617,102 @@ func (gs *GormStore) clearIndices() {
gs.indices.Clear()
}
+// persistIndexEntries writes index entries for a resource to the database
+// If oldResource is not nil, first deletes old entries, then inserts new ones
+func (gs *GormStore) persistIndexEntries(resource model.Resource, oldResource
model.Resource) error {
+ db := gs.pool.GetDB()
+
+ // Delete old entries if updating
+ if oldResource != nil {
+ if err := db.Where("resource_key = ?",
oldResource.ResourceKey()).Delete(&ResourceIndexModel{}).Error; err != nil {
+ return err
+ }
+ }
+
+ // Get all index entries for this resource
+ indexers := gs.indices.GetIndexers()
+ var entries []ResourceIndexModel
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ for _, v := range values {
+ entries = append(entries, ResourceIndexModel{
+ ResourceKind: gs.kind.ToString(),
+ IndexName: indexName,
+ IndexValue: v,
+ ResourceKey: resource.ResourceKey(),
+ })
+ }
+ }
+
+ if len(entries) == 0 {
+ return nil
+ }
+
+ return db.Create(&entries).Error
+}
+
+// deleteIndexEntries removes all index entries for a resource key
+func (gs *GormStore) deleteIndexEntries(resourceKey string) error {
+ db := gs.pool.GetDB()
+ return db.Where("resource_key = ?",
resourceKey).Delete(&ResourceIndexModel{}).Error
+}
+
+// getKeysByPrefixFromDB retrieves resource keys matching a prefix from the
database
+func (gs *GormStore) getKeysByPrefixFromDB(indexName, prefix string)
([]string, error) {
+ db := gs.pool.GetDB()
+ var entries []ResourceIndexModel
+ err := db.Where("resource_kind = ? AND index_name = ? AND index_value
LIKE ?",
+ gs.kind.ToString(), indexName, prefix+"%").
+ Find(&entries).Error
+ if err != nil {
+ return nil, err
+ }
+
+ keys := make([]string, 0, len(entries))
+ seen := make(map[string]struct{})
+ for _, e := range entries {
+ if _, ok := seen[e.ResourceKey]; !ok {
+ keys = append(keys, e.ResourceKey)
+ seen[e.ResourceKey] = struct{}{}
+ }
+ }
+ return keys, nil
+}
+
// rebuildIndices rebuilds all in-memory indices from existing database records
-// This is called during initialization to ensure indices are populated with
existing data
+// First tries to rebuild from resource_indices table for efficiency
+// Falls back to rebuilding from resource table if resource_indices is empty
func (gs *GormStore) rebuildIndices() error {
// Clear existing indices first
gs.clearIndices()
+ // Try to load from resource_indices table (persisted indices)
+ var entries []ResourceIndexModel
+ db := gs.pool.GetDB()
+ if err := db.Where("resource_kind = ?",
gs.kind.ToString()).Find(&entries).Error; err != nil {
+ logger.Warnf("failed to load from resource_indices for %s: %v",
gs.kind.ToString(), err)
+ return gs.rebuildIndicesFromResources()
+ }
+
+ // If resource_indices table has entries, rebuild from there
+ if len(entries) > 0 {
+ for _, entry := range entries {
+ gs.indices.AddEntry(entry.IndexName, entry.IndexValue,
entry.ResourceKey)
+ }
+ logger.Infof("Rebuilt indices for %s from resource_indices
table: %d entries", gs.kind.ToString(), len(entries))
+ return nil
+ }
+
+ // Fallback: rebuild from resources table
Review Comment:
`rebuildIndices` rebuilds from `resource_indices` as soon as it finds any
rows for the kind. If indexers have changed (new index added) or the table is
only partially populated, this will skip rebuilding missing indexes from the
resource table, leaving queries incomplete. Consider validating that required
index names are present (or versioning the index schema) and falling back to
`rebuildIndicesFromResources` when coverage is incomplete.
```suggestion
// Always rebuild from the resources table to ensure indices are
complete
// and compatible with the current indexers. This will also repopulate
// the resource_indices table via persistIndexEntries.
```
##########
pkg/store/memory/store.go:
##########
@@ -204,3 +286,77 @@ func (rs *resourceStore) getKeysByIndexes(indexes
map[string]string) ([]string,
}
return keySet.ToSlice(), nil
}
+
+// addToTrees adds a resource to all relevant RadixTrees for prefix matching
+func (rs *resourceStore) addToTrees(resource coremodel.Resource) {
+ rs.treesMu.Lock()
+ defer rs.treesMu.Unlock()
+
+ // Get indexers from storeProxy, not from global registry
+ // This ensures we include both init-time and dynamically-added indexers
+ indexers := rs.storeProxy.GetIndexers()
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ tree, ok := rs.prefixTrees[indexName]
+ if !ok || tree == nil {
+ continue
+ }
+ for _, v := range values {
+ // Key format: "indexValue/resourceKey"
+ key := v + "/" + resource.ResourceKey()
+ tree.Insert(key, struct{}{})
+ }
Review Comment:
The RadixTree key is built as `indexValue + "/" + resource.ResourceKey()`,
but `ResourceKey()` is typically `mesh/name` (contains `/`). This makes the
delimiter ambiguous and breaks prefix lookups (and deletes) whenever resource
keys contain `/`. Consider storing the resourceKey(s) in the tree value keyed
by the indexValue (or use an unambiguous encoding/delimiter) so the original
full resource key can be recovered reliably.
##########
pkg/store/memory/store_test.go:
##########
@@ -784,3 +785,425 @@ func TestResourceStore_ListIndexFuncValues(t *testing.T) {
assert.Contains(t, values, "active")
assert.Contains(t, values, "inactive")
}
+
+func TestResourceStore_HasPrefixMatch(t *testing.T) {
+ store := NewMemoryResourceStore("TestInstance")
+ err := store.Init(nil)
+ assert.NoError(t, err)
+
+ // Create mock resources with IP-like values for prefix matching
+ mockRes1 := &mockResource{
+ kind: "TestInstance",
+ key: "instance-1",
+ mesh: "default",
+ meta: metav1.ObjectMeta{
+ Name: "instance-1",
+ Labels: map[string]string{
+ "ip": "192.168.1.10",
+ },
+ },
Review Comment:
These HasPrefix tests use resource keys like "instance-1" that don't contain
the path separator. In production `ResourceKey()` is typically `mesh/name`, so
keys include `/`; using a realistic key format here would catch issues where
prefix indexing/parsing mishandles `/` in resource keys.
##########
go.mod:
##########
@@ -110,8 +96,9 @@ require (
)
require (
- cel.dev/expr v0.23.0 // indirect
filippo.io/edwards25519 v1.1.0 // indirect
+ github.com/BurntSushi/toml v1.3.2 // indirect
+ github.com/armon/go-radix v1.0.0 // indirect
Review Comment:
`github.com/armon/go-radix` is imported directly from non-test code
(`pkg/store/memory/store.go`), but in go.mod it is listed with `// indirect`.
Running `go mod tidy` (or moving it to the direct require block) should make
the dependency classification accurate.
##########
pkg/store/memory/store.go:
##########
@@ -204,3 +286,77 @@ func (rs *resourceStore) getKeysByIndexes(indexes
map[string]string) ([]string,
}
return keySet.ToSlice(), nil
}
+
+// addToTrees adds a resource to all relevant RadixTrees for prefix matching
+func (rs *resourceStore) addToTrees(resource coremodel.Resource) {
+ rs.treesMu.Lock()
+ defer rs.treesMu.Unlock()
+
+ // Get indexers from storeProxy, not from global registry
+ // This ensures we include both init-time and dynamically-added indexers
+ indexers := rs.storeProxy.GetIndexers()
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ tree, ok := rs.prefixTrees[indexName]
+ if !ok || tree == nil {
+ continue
+ }
+ for _, v := range values {
+ // Key format: "indexValue/resourceKey"
+ key := v + "/" + resource.ResourceKey()
+ tree.Insert(key, struct{}{})
+ }
+ }
+}
+
+// removeFromTrees removes a resource from all relevant RadixTrees
+func (rs *resourceStore) removeFromTrees(resource coremodel.Resource) {
+ rs.treesMu.Lock()
+ defer rs.treesMu.Unlock()
+
+ // Get indexers from storeProxy, not from global registry
+ // This ensures we include both init-time and dynamically-added indexers
+ indexers := rs.storeProxy.GetIndexers()
+ for indexName, indexFunc := range indexers {
+ values, err := indexFunc(resource)
+ if err != nil {
+ continue
+ }
+ tree, ok := rs.prefixTrees[indexName]
+ if !ok || tree == nil {
+ continue
+ }
+ for _, v := range values {
+ // Key format: "indexValue/resourceKey"
+ key := v + "/" + resource.ResourceKey()
+ tree.Delete(key)
+ }
+ }
+}
+
+// getKeysByPrefix retrieves resource keys by prefix match using RadixTree
+func (rs *resourceStore) getKeysByPrefix(indexName, prefix string) ([]string,
error) {
+ rs.treesMu.RLock()
+ defer rs.treesMu.RUnlock()
+
+ tree, ok := rs.prefixTrees[indexName]
+ if !ok {
+ return nil, fmt.Errorf("index %s does not exist", indexName)
+ }
+
+ var keys []string
+ tree.WalkPrefix(prefix, func(k string, v interface{}) bool {
+ // Key format: "indexValue/resourceKey"
+ // Extract the resourceKey part (after the last "/")
+ idx := strings.LastIndex(k, "/")
+ if idx >= 0 && idx < len(k)-1 {
+ keys = append(keys, k[idx+1:])
+ }
Review Comment:
`getKeysByPrefix` extracts the resource key using `strings.LastIndex(k,
"/")`, which will truncate keys like `mesh/name` to just `name`. Since
`GetByKey` expects the full resource key, HasPrefix queries can return keys
that will never resolve to stored resources. The tree should store the full
resource key without needing to parse it back out of the string key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
