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/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
