2 - Broadcast - Datatypes by aleksander-vedvik · Pull Request #199 · relab/gorums

aleksander-vedvik · 2025-01-11T13:38:46Z

This pull request build on top of: #198

This is a preparation pr that includes the most important types in the broadcast implementation.

Data types, such as dtos and errors, used in the broadcast implementation

meling

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.

meling · 2025-01-11T19:06:19Z

broadcast/dtos/dtos.go

@@ -0,0 +1,75 @@
+package dtos


We need a package doc comment for dtos to explain what it is and provides.

Maybe the package name could be more specific. I can't figure out what it means, so I can't think of something better.

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).

meling · 2025-01-11T19:40:17Z

broadcast/errors/errors.go

@@ -0,0 +1,57 @@
+package broadcastErrors


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.

Note also: Since the package is declared as broadcastErrors but the file is saved in the errors folder, this will not compile.

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.

meling · 2025-01-11T19:43:05Z

broadcast/errors/errors.go

@@ -0,0 +1,57 @@
+package broadcastErrors
+
+type BroadcastIDErr struct{}


Rename it to IDErr to avoid stuttering.

Usage from outside the broadcast package would be broadcast.IDErr instead of broadcast.BroadcastIDErr.

meling · 2025-01-11T19:44:58Z

broadcast/errors/errors.go

+type MissingClientReqErr struct{}
+
+func (err MissingClientReqErr) Error() string {
+	return "has not received client req yet"


return "client request not received (yet)"

meling · 2025-01-11T19:49:17Z

broadcast/errors/errors.go

+type ClientReqAlreadyReceivedErr struct{}
+
+func (err ClientReqAlreadyReceivedErr) Error() string {
+	return "the client req has already been received. The forward req is thus dropped."


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)"

I like number 2 the best. 👍

meling · 2025-01-11T20:11:15Z

broadcast/errors/errors.go

+}
+
+func (err InvalidAddrErr) Error() string {
+	return "provided Addr is invalid. got: " + err.Addr


return fmt.Sprintf("invalid address: %s", err.Addr)

meling · 2025-01-11T20:13:45Z

broadcast/dtos/dtos.go

+	String() string
+}
+
+type BroadcastMsg struct {


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.

meling · 2025-01-11T20:33:24Z

broadcast/dtos/dtos.go

+}
+
+type BroadcastMsg struct {
+	Ctx        context.Context


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.

meling · 2025-01-11T20:35:20Z

broadcast/errors/errors.go

+	return "wrong broadcastID"
+}
+
+type MissingClientReqErr struct{}


Would be nice to have doc comments on these errors too.

meling · 2025-01-11T20:40:14Z

broadcast/dtos/dtos.go

+	Close   func() error
+}
+
+type BroadcastOptions struct {


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?

Yes, they are. I provided a longer explanation to one of the comments above. 👍

aleksander-vedvik · 2025-01-12T10:18:26Z

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.

Yes, please do. I'll instead update the subsequent PR's with the changes. 👍

aleksander-vedvik · 2025-01-18T12:33:13Z

Most of the suggestions have been fixed. However, some requires major refactor (such as moving the errors). Is it possible to leave them as they are, and instead fix them in later pull requests?

feat(Broadcast): required datatypes

6804a92

Data types, such as dtos and errors, used in the broadcast implementation

aleksander-vedvik added the enhancement label Jan 11, 2025

aleksander-vedvik requested a review from meling January 11, 2025 13:38

aleksander-vedvik changed the title ~~Features/broadcast/datatypes 2~~ Broadcast Datatypes Jan 11, 2025

aleksander-vedvik changed the title ~~Broadcast Datatypes~~ 2 - Broadcast - Datatypes Jan 11, 2025

aleksander-vedvik mentioned this pull request Jan 11, 2025

3 - Broadcast - Router #200

Open

meling changed the base branch from master to features/broadcast/logging January 11, 2025 18:59

meling requested changes Jan 11, 2025

View reviewed changes

aleksander-vedvik added 2 commits January 18, 2025 13:19

refactor(Broadcast): removed unused errs and added docs

38a909a

docs(Broadcast): documentation for broadcast dtos

45b93a3

refactor(Broadcast): broadcast prefix for errors

f94e935

		@@ -0,0 +1,57 @@
		package broadcastErrors

		type BroadcastIDErr struct{}

Conversation

aleksander-vedvik commented Jan 11, 2025

Uh oh!

meling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksander-vedvik commented Jan 12, 2025

Uh oh!

aleksander-vedvik commented Jan 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants