added sync runner and refactored operator creation#171
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| // WithServerOptions appends gRPC server options (interceptors, TLS | ||
| // credentials, etc.) used when the underlying grpc.Server is created. | ||
| func WithServerOptions(opts ...grpc.ServerOption) ServerOption { |
There was a problem hiding this comment.
grpc.WithServerOptions(opts ...GRPC.ServerOption) grpc.ServerOption
I think the pkg naming isn't optimal because we now have 2 grpc pkgs using the same name. So using this function requires one to make aliased imports.
What if we call this pkg rpc? Its a similar case like the AMQP and even if it weren't, I think its still cleaner.
| // Processor handles signature verification, handler dispatch, and | ||
| // response signing. It is transport-agnostic: a Runner or server | ||
| // calls Processor.Process for each inbound TaskRequest. | ||
| Processor struct { |
NamanBalaji
force-pushed
the
feat/sync-operator-runner
branch
from
ee8895a to
20bf73e
Compare
fabenan-f
left a comment
fabenan-f
left a comment
There was a problem hiding this comment.
just a few comments to consider
There was a problem hiding this comment.
Since this directory evolved quite a bit, we could think of renaming it from client to transport or something else (probable better in a separate PR if at all)
| } | ||
| } | ||
|
|
||
| // Run registers the TaskService, starts serving, and blocks until |
There was a problem hiding this comment.
Run can be called multiple times but stop only once, so the second server cannot be gracefully stopped. This might not happen when used but then again there is Murphy's Law
There was a problem hiding this comment.
Fixed it so that Run and stop can only be called once on a grpc client.
| // Signer and Verifier for signing and verification operations. | ||
| type TargetOperator struct { | ||
| Runner Runner | ||
| Client Responder |
There was a problem hiding this comment.
I think calling the field Responder is more accurate even though it is inconsistent with TargetManager (I'd then rather change the field name there, too, probably in a separate PR)
There was a problem hiding this comment.
Yes, naming could be better. I can create a subsequent refactoring PR
| // shutdown. | ||
| Runner interface { | ||
| Run(ctx context.Context, process ProcessFunc) | ||
| // TaskRequestHandler is the processing pipeline handed to a Responder. |
There was a problem hiding this comment.
np but I think important for the understanding: The TaskRequestHandler is handed to a SyncResponder and automatically called by an AsyncResponder
|
|
||
| // TargetOperator holds the runner and cryptographic implementation for responding | ||
| // Responder is the base interface for any transport that can be closed. | ||
| type Responder interface { |
There was a problem hiding this comment.
I'd rather keep Responder just as a speaking type without any method definition than having this misplaced (imho) Close here. Otherwise, we could define it as a constraint like
type Responder interface {
SyncResponder | AsyncResponder
}
and go down the generic rabbit hole but I'm not sure if this is a good idea
There was a problem hiding this comment.
Yes if we add a union constraint we can't type switch in the operator anymore and then we would need
type TargetOperator[R SyncResponder | AsyncResponder] struct {
Client R
}
type Operator[R SyncResponder | AsyncResponder] struct { ... }
as go doesn't allow switching on type parameter.
4 participants
No description provided.