Internal redispatch #39

jakedt · 2021-08-27T16:05:42Z

No description provided.

josephschorr · 2021-08-27T16:21:26Z

.github/workflows/build.yaml

@@ -71,9 +71,11 @@ jobs:
    - name: "Generate & Diff Servok Protos"
      run: |
        ./buf.gen.yaml
+        git diff


Why these additions? Just to make the errors nicer?

Yeah it was impossible for me to see what the difference was.

josephschorr · 2021-08-27T16:21:53Z

cmd/spicedb/root.go

@@ -77,8 +82,11 @@ func newRootCmd() *cobra.Command {
 	// Flags for parsing and validating schemas.
 	rootCmd.Flags().Bool("schema-prefixes-required", false, "require prefixes on all object definitions in schemas")

+	// Flags for internal dispatch API
+	rootCmd.Flags().String("internal-grpc-addr", ":50052", "address to listen for internal requests")


Since we use 50052 for corrino currently, can we default to this something like 50053?

everything uses 50051 now (inherited from cobrautil), but we might want to make this port totally different so that there's no mistaking it

josephschorr · 2021-08-27T16:24:56Z

internal/dispatch/caching/caching.go

+	}
+
+	totalCounter := prometheus.NewCounter(prometheus.CounterOpts{
+		Namespace: "spicedb",


Move the "spicedb" into a constant

josephschorr · 2021-08-27T16:25:50Z

internal/dispatch/caching/caching.go

+
+// DispatchExpand implements dispatch.Expand interface
+func (cd *cachingDispatcher) DispatchExpand(ctx context.Context, req *v1.DispatchExpandRequest) (*v1.DispatchExpandResponse, error) {
+	return cd.d.DispatchExpand(ctx, req)


Add comments here and in Lookup that we're not doing caching yet

josephschorr · 2021-08-27T16:26:45Z

internal/dispatch/caching/caching.go

+		return nil, fmt.Errorf(errCachingInitialization, err)
+	}
+
+	totalCounter := prometheus.NewCounter(prometheus.CounterOpts{


Probably should name this differently, since I'd expect it to be for all api calls, not just checks

josephschorr · 2021-08-27T16:28:04Z

internal/dispatch/caching/caching.go

+		cd.c.Set(requestKey, toCache, checkResultEntryCost)
+	}
+
+	return computed, err


Maybe be explicit here in returning nil for the result?

It's not always nil here.

We have a non-nil result when there is an error?

the error condition doesn't short circuit

Oh... that's highly unexpected to me. Maybe just return the error if it occurs and move the cache update outside of the branch?

yeah i think adding the short circuit would make this less prone to getting broken in a future refactor

josephschorr · 2021-08-27T16:30:08Z

internal/dispatch/graph/check_test.go

+					})
+
+					require.NoError(err)
+					require.Equal(expected.isMember, checkResult.Membership == v1.DispatchCheckResponse_MEMBER)


Perhaps add a dispatch again and verify again, to ensure caching is working as expected?

which caching? This only has namespace manager caching, not dispatch caching.

jzelinskie

Sorry a bunch of my comments were on early commits from before refactors and then I just decided to jump ahead to the final state.

jzelinskie · 2021-08-30T16:53:25Z

pkg/smartclient/v2/client.go

+		sync.Mutex{},
+		consistent.NewHashring(xxhash.Sum64, hashringReplicationFactor),
+		cancel,
+		proto.MarshalOptions{Deterministic: true},


Is there a time we don't want this to be deterministic? Why bother parameterizing it?

jzelinskie · 2021-08-30T17:03:01Z

pkg/smartclient/v2/client.go

+		}
+
+		for ctx.Err() == nil {
+			endpointResponse, err := stream.Recv()


isn't the first response always an empty one? (i'm remembering this from an earlier iteration of this code)

I fixed that. It was a disaster when servok restarted.

jzelinskie · 2021-08-30T17:15:45Z

pkg/smartclient/v2/client.go

+}
+
+// Stop will cancel the client watch and clean up the pool
+func (sc *SmartClient) Stop() {


It might be more idiomatic to support context cancellation for this rather than an explicit Stop function.
I can see arguments both ways, but I think we should strive to use one cohesive strategy across the codebase.

jzelinskie · 2021-08-30T17:32:49Z

pkg/smartclient/v2/client.go

+
+var errNoBackends = errors.New("no backends available for request")
+
+// SmartClient is a client which utilizes a dynamic source of backends and a consistent


I know it's a nit, but I really dislike "smart client" -- it doesn't really mean anything and gives absolutely no information about the behavior of the client other than it has "some kind of logic".

LiveHashringDispatchClient or UpdatingHashringDispatchClient maybe would be better?

jzelinskie · 2021-08-30T18:13:13Z

pkg/smartclient/v2/options.go

+	"github.com/authzed/spicedb/pkg/x509util"
+	"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials"


import mixing

internal/graph/lookup.go

jzelinskie · 2021-08-30T19:11:57Z

internal/dispatch/caching/caching.go

+		cd.c.Set(requestKey, toCache, checkResultEntryCost)
+	}
+
+	return computed, err


yeah i think adding the short circuit would make this less prone to getting broken in a future refactor

jzelinskie · 2021-08-30T19:15:01Z

internal/dispatch/graph/graph.go

+var tracer = otel.Tracer("spicedb/internal/dispatch/local")
+
+// NewLocalOnlyDispatcher creates a dispatcher that consults with the graph to formulate a response.
+func NewLocalOnlyDispatcher(


is there a reason to call this LocalOnly rather than just Local?

Yes, because a "local" dispatcher can be configured to redispatch to the network.

that's not at all obvious from the names

jzelinskie · 2021-08-30T19:16:27Z

internal/dispatch/remote/cluster.go

+	v1 "github.com/authzed/spicedb/internal/proto/dispatch/v1"
+)
+
+type clusterClient interface {


dispatchClient

jzelinskie · 2021-08-30T19:20:00Z

internal/dispatch/remote/cluster.go

+
+// NewClusterDispatcher creates a dispatcher implementation that uses the provided client
+// to dispatch requests to peer nodes in the cluster.
+func NewClusterDispatcher(client clusterClient) dispatch.Dispatcher {


I get it -- but this API is kinda weird since you're creating a dispatcher from a dispatcher.
Maybe we can make this more specific and name this one the DepthCheckingDispatcher?

Whoa that's a gross misrepresentation of what this does!

I'm pretty sure I'm just crazy overloading the term "dispatcher"

josephschorr · 2021-08-31T18:31:11Z

internal/dispatch/caching/caching.go

+		cd.c.Set(requestKey, toCache, checkResultEntryCost)
+	}
+
+	// Return both the computed and err in ALL cases, computed contains resolved metadata even


jzelinskie

looks good to me after a rebase

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

jakedt requested review from jzelinskie and josephschorr August 27, 2021 16:05

github-actions bot added area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Aug 27, 2021

jakedt force-pushed the internal-redispatch branch from 18e3edd to c33c5a2 Compare August 27, 2021 16:07

josephschorr reviewed Aug 27, 2021

View reviewed changes

jzelinskie added the priority/2 medium This needs to be done label Aug 27, 2021

jakedt force-pushed the internal-redispatch branch from fa34750 to 94baf04 Compare August 30, 2021 15:23

jzelinskie requested changes Aug 30, 2021

View reviewed changes

josephschorr reviewed Aug 31, 2021

View reviewed changes

jzelinskie approved these changes Aug 31, 2021

View reviewed changes

jakedt added 16 commits August 31, 2021 15:52

spicedb: add an internal redispatch API

544bd55

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

spicedb: add an internal API smartclient

84d98d8

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

spicedb: use the internal API everywhere

aabd3ea

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

split and refactor graph and dispatch

463146a

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

remove the tracer code that's no longer used

7ce8610

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

show the contents of the git diff for protos

0576b25

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

better lookup request logging

55f01d6

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

rename the prom metrics variables in caching dispatch

caa3295

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

change the internal grpc port

74d2ce4

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

log whether an internal expand was recursive

2f8f799

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

rename smartclient to consistent backend client

f7e8eaf

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

document the lookaside cache handling

1408a4c

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

remove the unnecessary short circuits

e91a506

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

make consistent backend client more idiomatic

e926286

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

rework the way the consistent backend client startup works

84488e0

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

handle more error and shutdown conditions on startup

0ae9a5c

Signed-off-by: Jake Moshenko <jacob.moshenko@gmail.com>

jakedt force-pushed the internal-redispatch branch from e1fce20 to 0ae9a5c Compare August 31, 2021 19:58

jakedt merged commit b7e2031 into main Aug 31, 2021

jakedt deleted the internal-redispatch branch August 31, 2021 20:02

jakedt mentioned this pull request Jun 6, 2022

Implement a dedicated gRPC API for dispatch #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal redispatch #39

Internal redispatch #39

jakedt commented Aug 27, 2021

josephschorr Aug 27, 2021

jakedt Aug 27, 2021

josephschorr Aug 27, 2021

jzelinskie Aug 30, 2021

josephschorr Aug 27, 2021

josephschorr Aug 27, 2021

josephschorr Aug 27, 2021

josephschorr Aug 27, 2021

jakedt Aug 27, 2021

josephschorr Aug 27, 2021

jakedt Aug 28, 2021

josephschorr Aug 30, 2021 •

edited

Loading

jzelinskie Aug 30, 2021

josephschorr Aug 27, 2021

jakedt Aug 28, 2021

jzelinskie left a comment

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jakedt Aug 31, 2021

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jakedt Aug 31, 2021

jzelinskie Aug 31, 2021

jzelinskie Aug 30, 2021

jzelinskie Aug 30, 2021

jakedt Aug 31, 2021

jzelinskie Aug 31, 2021

josephschorr Aug 31, 2021

jzelinskie left a comment


		var errNoBackends = errors.New("no backends available for request")

		// SmartClient is a client which utilizes a dynamic source of backends and a consistent

Internal redispatch #39

Internal redispatch #39

Conversation

jakedt commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzelinskie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzelinskie left a comment

Choose a reason for hiding this comment

josephschorr Aug 30, 2021 •

edited

Loading