syncbase/server: Implement GlobChildren__ instead of Glob__.
The client-side Glob implementation is now simpler, since it can use the
namespace.Glob method directly.
GlobChildren__ is much simpler than Glob__, and gives us (almost)
exactly the behavior we want. It can be problematic when a name has
lots of children, because all the children must be returned by
GlobChildren__, and filtering happens at a higher level. We should
consider passing a pattern to GlobChildren__ so that it can choose to do
its own filtering to save work.
Change-Id: I094b07cc039fcfd9c14d0e92ef2e7ea1223be80f
diff --git a/v23/syncbase/util/glob.go b/v23/syncbase/util/glob.go
index 3badbe9..b4f4201 100644
--- a/v23/syncbase/util/glob.go
+++ b/v23/syncbase/util/glob.go
@@ -5,46 +5,38 @@
package util
import (
- "io"
"sort"
+ "strings"
"v.io/v23"
"v.io/v23/context"
"v.io/v23/naming"
- "v.io/v23/rpc"
)
-// List does client.Glob("*") and returns a sorted slice of results or a
-// VDL-compatible error.
+// List does namespace.Glob("name/*") and returns a sorted slice of results or
+// a VDL-compatible error.
func List(ctx *context.T, name string) ([]string, error) {
- client := v23.GetClient(ctx)
- // TODO(sadovsky): This is nuts. Why is Glob not a method on the stub, just
- // like every other streaming method?
- call, err := client.StartCall(ctx, name, rpc.GlobMethod, []interface{}{"*"})
+ ns := v23.GetNamespace(ctx)
+ ch, err := ns.Glob(ctx, naming.Join(name, "*"))
if err != nil {
return nil, err
}
- res := []string{}
- done := false
- for !done {
- var gr naming.GlobReply
- switch err := call.Recv(&gr); err {
- case nil:
- switch v := gr.(type) {
- case naming.GlobReplyEntry:
- res = append(res, v.Value.Name)
- case naming.GlobReplyError:
- return nil, v.Value.Error
- }
- case io.EOF:
- done = true
- default:
- return nil, err
+
+ names := []string{}
+ for globReply := range ch {
+ switch v := globReply.(type) {
+ case *naming.GlobReplyEntry:
+ // NOTE(nlacasse): The names that come back from Glob are all
+ // rooted. We only want the last part of the name, so we must chop
+ // off everything before the final '/'. Since endpoints can
+ // themselves contain slashes, we have to remove the endpoint from
+ // the name first.
+ _, name := naming.SplitAddressName(v.Value.Name)
+ names = append(names, name[strings.LastIndex(name, "/")+1:])
+ case *naming.GlobReplyError:
+ return nil, v.Value.Error
}
}
- if err := call.Finish(); err != nil {
- return nil, err
- }
- sort.Strings(res)
- return res, nil
+ sort.Strings(names)
+ return names, nil
}
diff --git a/x/ref/services/syncbase/server/app.go b/x/ref/services/syncbase/server/app.go
index 81276f0..2151b1a 100644
--- a/x/ref/services/syncbase/server/app.go
+++ b/x/ref/services/syncbase/server/app.go
@@ -13,7 +13,6 @@
"v.io/syncbase/x/ref/services/syncbase/server/util"
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
- "v.io/v23/naming"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -63,13 +62,14 @@
return data.Perms, util.FormatVersion(data.Version), nil
}
-func (a *app) Glob__(ctx *context.T, call rpc.ServerCall, pattern string) (<-chan naming.GlobReply, error) {
+func (a *app) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
// Check perms.
sn := a.s.st.NewSnapshot()
if err := util.Get(ctx, call, sn, a, &appData{}); err != nil {
sn.Close()
return nil, err
}
+ pattern := "*"
return util.Glob(ctx, call, pattern, sn, util.JoinKeyParts(util.DbInfoPrefix, a.name))
}
diff --git a/x/ref/services/syncbase/server/nosql/database.go b/x/ref/services/syncbase/server/nosql/database.go
index 94664fb..c69af62 100644
--- a/x/ref/services/syncbase/server/nosql/database.go
+++ b/x/ref/services/syncbase/server/nosql/database.go
@@ -12,7 +12,6 @@
"v.io/syncbase/x/ref/services/syncbase/store/memstore"
"v.io/syncbase/x/ref/services/syncbase/store/watchable"
"v.io/v23/context"
- "v.io/v23/naming"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -97,13 +96,14 @@
return data.Perms, util.FormatVersion(data.Version), nil
}
-func (d *database) Glob__(ctx *context.T, call rpc.ServerCall, pattern string) (<-chan naming.GlobReply, error) {
+func (d *database) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
// Check perms.
sn := d.st.NewSnapshot()
if err := util.Get(ctx, call, sn, d, &databaseData{}); err != nil {
sn.Close()
return nil, err
}
+ pattern := "*"
return util.Glob(ctx, call, pattern, sn, util.TablePrefix)
}
diff --git a/x/ref/services/syncbase/server/nosql/table.go b/x/ref/services/syncbase/server/nosql/table.go
index edfd195..ca89bcf 100644
--- a/x/ref/services/syncbase/server/nosql/table.go
+++ b/x/ref/services/syncbase/server/nosql/table.go
@@ -9,7 +9,6 @@
"v.io/syncbase/x/ref/services/syncbase/server/util"
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
- "v.io/v23/naming"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -117,13 +116,14 @@
return verror.NewErrNotImplemented(ctx)
}
-func (t *table) Glob__(ctx *context.T, call rpc.ServerCall, pattern string) (<-chan naming.GlobReply, error) {
+func (t *table) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
// Check perms.
sn := t.d.st.NewSnapshot()
if err := util.Get(ctx, call, sn, t, &tableData{}); err != nil {
sn.Close()
return nil, err
}
+ pattern := "*"
return util.Glob(ctx, call, pattern, sn, util.JoinKeyParts(util.RowPrefix, t.name))
}
diff --git a/x/ref/services/syncbase/server/service.go b/x/ref/services/syncbase/server/service.go
index 68a72ab..ce2c63e 100644
--- a/x/ref/services/syncbase/server/service.go
+++ b/x/ref/services/syncbase/server/service.go
@@ -14,7 +14,6 @@
"v.io/syncbase/x/ref/services/syncbase/store/memstore"
"v.io/syncbase/x/ref/services/syncbase/vsync"
"v.io/v23/context"
- "v.io/v23/naming"
"v.io/v23/rpc"
"v.io/v23/security/access"
"v.io/v23/verror"
@@ -87,13 +86,14 @@
return data.Perms, util.FormatVersion(data.Version), nil
}
-func (s *service) Glob__(ctx *context.T, call rpc.ServerCall, pattern string) (<-chan naming.GlobReply, error) {
+func (s *service) GlobChildren__(ctx *context.T, call rpc.ServerCall) (<-chan string, error) {
// Check perms.
sn := s.st.NewSnapshot()
if err := util.Get(ctx, call, sn, s, &serviceData{}); err != nil {
sn.Close()
return nil, err
}
+ pattern := "*"
return util.Glob(ctx, call, pattern, sn, util.AppPrefix)
}
diff --git a/x/ref/services/syncbase/server/util/glob.go b/x/ref/services/syncbase/server/util/glob.go
index fe78670..0feb974 100644
--- a/x/ref/services/syncbase/server/util/glob.go
+++ b/x/ref/services/syncbase/server/util/glob.go
@@ -9,13 +9,28 @@
"v.io/syncbase/x/ref/services/syncbase/store"
"v.io/v23/context"
- "v.io/v23/naming"
"v.io/v23/rpc"
"v.io/v23/verror"
"v.io/x/lib/vlog"
"v.io/x/ref/lib/glob"
)
+// NOTE(nlacasse): Syncbase handles Glob requests by implementing
+// GlobChildren__ at each level (service, app, database, table).
+//
+// Each GlobChildren__ method returns all the children of the name without
+// filtering them in any way. Any filtering that needs to happen based on the
+// glob pattern happens magically at a higher level. This results in a simple
+// GlobChildren__ implementation, but often an inefficient one, since we must
+// return every single child name, even though many will be filtered before
+// being sent to the client.
+//
+// We should consider changing GlobChildren__ so that it takes the pattern that
+// is being Globbed as an argument. Then, GlobChildren__ could be smarter about
+// what it returns, and only return names that already match the pattern. This
+// would prevent us from having to read all child names from the database every
+// time we Glob.
+
// globPatternToPrefix takes "foo*" and returns "foo".
// It assumes the input pattern is a valid glob pattern, and returns
// verror.ErrBadArg if the input is not a valid prefix.
@@ -42,7 +57,7 @@
// TODO(sadovsky): It sucks that Glob must be implemented differently from other
// streaming RPC handlers. I don't have much confidence that I've implemented
// both types of streaming correctly.
-func Glob(ctx *context.T, call rpc.ServerCall, pattern string, sn store.Snapshot, stKeyPrefix string) (<-chan naming.GlobReply, error) {
+func Glob(ctx *context.T, call rpc.ServerCall, pattern string, sn store.Snapshot, stKeyPrefix string) (<-chan string, error) {
// TODO(sadovsky): Support glob with non-prefix pattern.
if _, err := glob.Parse(pattern); err != nil {
sn.Close()
@@ -57,7 +72,7 @@
return nil, verror.New(verror.ErrInternal, ctx, err)
}
it := sn.Scan(ScanPrefixArgs(stKeyPrefix, prefix))
- ch := make(chan naming.GlobReply)
+ ch := make(chan string)
go func() {
defer sn.Close()
defer close(ch)
@@ -65,13 +80,14 @@
for it.Advance() {
key = it.Key(key)
parts := SplitKeyParts(string(key))
- ch <- naming.GlobReplyEntry{naming.MountEntry{Name: parts[len(parts)-1]}}
+ ch <- parts[len(parts)-1]
}
if err := it.Err(); err != nil {
+ // TODO(nlacasse): We should do something with the error other than
+ // just logging. Ideally we could send the error back to the
+ // client, but the GlobChildren__ API doesn't really allow for that
+ // since it uses a channel of strings rather than GlobResults.
vlog.VI(1).Infof("Glob(%q) failed: %v", pattern, err)
- ch <- naming.GlobReplyError{naming.GlobError{
- Error: verror.New(verror.ErrInternal, ctx, err),
- }}
}
}()
return ch, nil
