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/services/syncbase/server/app.go b/services/syncbase/server/app.go
index 81276f0..2151b1a 100644
--- a/services/syncbase/server/app.go
+++ b/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/services/syncbase/server/nosql/database.go b/services/syncbase/server/nosql/database.go
index 94664fb..c69af62 100644
--- a/services/syncbase/server/nosql/database.go
+++ b/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/services/syncbase/server/nosql/table.go b/services/syncbase/server/nosql/table.go
index edfd195..ca89bcf1 100644
--- a/services/syncbase/server/nosql/table.go
+++ b/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/services/syncbase/server/service.go b/services/syncbase/server/service.go
index 68a72ab..ce2c63e 100644
--- a/services/syncbase/server/service.go
+++ b/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/services/syncbase/server/util/glob.go b/services/syncbase/server/util/glob.go
index fe78670..0feb974 100644
--- a/services/syncbase/server/util/glob.go
+++ b/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
