2 - Broadcast - Datatypes #199
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package dtos | ||
|
|
||
| import ( | ||
| "context" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| "time" | ||
| ) | ||
|
|
||
| type Msg interface { | ||
| GetBroadcastID() uint64 | ||
| GetMethod() string | ||
| String() string | ||
| } | ||
|
|
||
| type BroadcastMsg struct { | ||
|
There was a problem hiding this comment. It would be quite useful with doc comments for structs, methods, fields etc. Especially, those fields that aren't common knowledge, such as |
||
| Ctx context.Context | ||
|
There was a problem hiding this comment. Difficult to assess, since I don't know the use of the |
||
| Options BroadcastOptions | ||
| OriginAddr string | ||
| Info Info | ||
| } | ||
|
|
||
| func (msg *BroadcastMsg) GetBroadcastID() uint64 { | ||
| return msg.Info.BroadcastID | ||
| } | ||
|
|
||
| func (msg *BroadcastMsg) GetMethod() string { | ||
| return msg.Info.Method | ||
| } | ||
|
|
||
| func (msg *BroadcastMsg) String() string { | ||
| return "broadcast" | ||
| } | ||
|
|
||
| type ReplyMsg struct { | ||
| Info Info | ||
| ClientAddr string | ||
| Err error | ||
| } | ||
|
|
||
| func (r *ReplyMsg) GetBroadcastID() uint64 { | ||
| return r.Info.BroadcastID | ||
| } | ||
|
|
||
| func (r *ReplyMsg) GetMethod() string { | ||
| return "reply" | ||
| } | ||
|
|
||
| func (r *ReplyMsg) String() string { | ||
| return "reply" | ||
| } | ||
|
|
||
| type Info struct { | ||
| Message protoreflect.ProtoMessage | ||
| BroadcastID uint64 | ||
| Method string | ||
| Addr string | ||
| OriginMethod string | ||
| OriginDigest []byte | ||
| OriginSignature []byte | ||
| OriginPubKey string | ||
| } | ||
|
|
||
| type Client struct { | ||
| Addr string | ||
| SendMsg func(timeout time.Duration, dto *ReplyMsg) error | ||
| Close func() error | ||
| } | ||
|
|
||
| type BroadcastOptions struct { | ||
|
There was a problem hiding this comment. This question applies to the all types and fields in the PR: Is it intentional that all types and fields are exported? Are they needed outside the There was a problem hiding this comment. Yes, they are. I provided a longer explanation to one of the comments above. 👍 |
||
| ServerAddresses []string | ||
| AllowDuplication bool | ||
| SkipSelf bool | ||
| ProgressTo string | ||
| RelatedToReq uint64 | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package broadcastErrors | ||
|
There was a problem hiding this comment. Go packages shouldn't use camel case. However, it is also uncommon to have errors in a separate package. Since I assume your errors are specific to broadcast, they should probably be part of the Some notable exceptions exist, e.g., upspin. Since Upspin is a large project, they standardize on a particular error style across the whole project. We could do the same, but perhaps that would be part of Asbjørn's thesis work. There was a problem hiding this comment. Note also: Since the package is declared as There was a problem hiding this comment. My bad. The package should've been renamed to errors. I agree that they should not be in their own package. I'm just not very happy with how the logging wrapper currently operates, as it pollutes the code a bit (see the log statements in processor.go). I was hoping to improve this my including more context/fields in each error and log the errors directly. Hopefully making the logging a bit less verbose and at the same time making the errors a bit more explicit. Since this is not implemented yet, it is probably better to move the errors to the packages that references them. |
||
|
|
||
| type BroadcastIDErr struct{} | ||
|
There was a problem hiding this comment. Rename it to Usage from outside the |
||
|
|
||
| func (err BroadcastIDErr) Error() string { | ||
| return "wrong broadcastID" | ||
|
There was a problem hiding this comment. General comment for all error strings. It can be easier to interpret errors if they are prefixed by the package name, e.g.,: return "broadcast: wrong broadcastID"
// OR unless there are many types IDs, maybe it would suffice:
return "broadcast: wrong ID" |
||
| } | ||
|
|
||
| type MissingClientReqErr struct{} | ||
|
There was a problem hiding this comment. Would be nice to have doc comments on these errors too. |
||
|
|
||
| func (err MissingClientReqErr) Error() string { | ||
| return "has not received client req yet" | ||
|
There was a problem hiding this comment. return "client request not received (yet)" |
||
| } | ||
|
|
||
| type AlreadyProcessedErr struct{} | ||
|
|
||
| func (err AlreadyProcessedErr) Error() string { | ||
| return "already processed request" | ||
|
There was a problem hiding this comment. return "request already processed"Should this be prefixed by |
||
| } | ||
|
|
||
| type ReqFinishedErr struct{} | ||
|
|
||
| func (err ReqFinishedErr) Error() string { | ||
| return "request has terminated" | ||
|
There was a problem hiding this comment. return "request terminated"Should it be prefixed by |
||
| } | ||
|
|
||
| type ClientReqAlreadyReceivedErr struct{} | ||
|
|
||
| func (err ClientReqAlreadyReceivedErr) Error() string { | ||
| return "the client req has already been received. The forward req is thus dropped." | ||
|
There was a problem hiding this comment. What about this: // The forwarded request is dropped since:
return "client request already received"
// OR:
return "client request already received (dropped)"
// OR:
return "forwarded client request already received (dropped)"There was a problem hiding this comment. I like number 2 the best. 👍 |
||
| } | ||
|
|
||
| type MissingReqErr struct{} | ||
|
|
||
| func (err MissingReqErr) Error() string { | ||
| return "a request has not been created yet." | ||
|
There was a problem hiding this comment. Not sure I understand when this could happen, but here is a shorter error message: // Add comment here with more context
return "request not created yet"Don't use |
||
| } | ||
|
|
||
| type OutOfOrderErr struct{} | ||
|
|
||
| func (err OutOfOrderErr) Error() string { | ||
| return "the message is out of order" | ||
|
There was a problem hiding this comment. return "request received out of order"Is this referring to a |
||
| } | ||
|
|
||
| type ShardDownErr struct{} | ||
|
|
||
| func (err ShardDownErr) Error() string { | ||
| return "the shard is down" | ||
|
There was a problem hiding this comment. // more context?
return "shard is down" |
||
| } | ||
|
|
||
| type InvalidAddrErr struct { | ||
| Addr string | ||
| } | ||
|
|
||
| func (err InvalidAddrErr) Error() string { | ||
| return "provided Addr is invalid. got: " + err.Addr | ||
|
There was a problem hiding this comment. return fmt.Sprintf("invalid address: %s", err.Addr) |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a package doc comment for
dtosto explain what it is and provides.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the package name could be more specific. I can't figure out what it means, so I can't think of something better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supposed to contain all dto's that are referenced outside of the broadcast package context (including subfolders). The main idea is to prevent cyclic imports and provide a common namespace for dto's pertaining to broadcast outside of the broadcast context. I.e. instead of referencing processor.RequestDto and router.Msg in gorums/server.go, I thought it would be easier to use dtos.RequestDto and dtos.Msg. Hoping this would reduce the cognitive load of developers working with higher level functionality such as nodes/channels, as both processor and router are implementation details specific to broadcasting.
Naming things are not my strong suit, so feel free to rename/change. If you think it is better, the dtos could also be moved to their respective packages (RequestDto -> processor, the rest -> router).