2 - Broadcast - Datatypes #199
Conversation
Data types, such as dtos and errors, used in the broadcast implementation
There was a problem hiding this comment.
So I did a rather thorough review here...
If you prefer that I push suggested changes directly to the PR, rather than asking you to fix my nitpicking comments. That way, I could instead ask only the questions that need more coordination and clarification. Let me know what's the best way forward.
| @@ -0,0 +1,75 @@ | |||
| package dtos | |||
There was a problem hiding this comment.
We need a package doc comment for dtos to explain what it is and provides.
There was a problem hiding this comment.
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.
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).
| @@ -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 broadcast package.
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 broadcastErrors but the file is saved in the errors folder, this will not compile.
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.
| @@ -0,0 +1,57 @@ | |||
| package broadcastErrors | |||
|
|
|||
| type BroadcastIDErr struct{} | |||
There was a problem hiding this comment.
Rename it to IDErr to avoid stuttering.
Usage from outside the broadcast package would be broadcast.IDErr instead of broadcast.BroadcastIDErr.
| type MissingClientReqErr struct{} | ||
|
|
||
| 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 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. 👍
| } | ||
|
|
||
| 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)| 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 Info, Options, OriginAddr.
| } | ||
|
|
||
| type BroadcastMsg struct { | ||
| Ctx context.Context |
There was a problem hiding this comment.
Difficult to assess, since I don't know the use of the Ctx field here, but it is a antipattern to include context.Context in structs. See the Context and Structs blog post. It should be passed as the first argument to all functions/methods that need a context. But see the blog for concrete advice.
| return "wrong broadcastID" | ||
| } | ||
|
|
||
| type MissingClientReqErr struct{} |
There was a problem hiding this comment.
Would be nice to have doc comments on these errors too.
| 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 broadcast/dtos package?
There was a problem hiding this comment.
Yes, they are. I provided a longer explanation to one of the comments above. 👍
Yes, please do. I'll instead update the subsequent PR's with the changes. 👍 |
This pull request build on top of: #198
This is a preparation pr that includes the most important types in the broadcast implementation.