Feature/#9 test dvoting #16

XioZ · 2024-05-30T23:04:15Z

Closing Test D-Voting with New Mino P2P #9
Rewrote Stream() to:
- have an orchestrator that relays messages among participants (participants can send/receive messages to/from the orchestrator as well as to/from other participants)
- wrap orchestrator's address to pass equality check by callers against protocol initiator's participant address
Passed integration check with d-voting (all actions from Initialize Nodes to See Results completed successfully)
Improved benchmark performance to slightly outperform gRPC

… session ends.

ineiti

Great work - I'm happy that it all fits together now. For the final report, please be sure to:

comment what you changed to improve the benchmark
describe what in the current API description is wrong / confusing

There are too many closures in the code, and it's not clear which closures capture which local variables. This is why I propose to create them as methods on the structure, and then call them like that. If we would have more time, it would also be easier to test them like that.

ineiti · 2024-05-31T09:24:20Z

mino/minows/address.go

+	} else {
+		addr, err = newOrchestratorAddr(location, identity)


Unreachable code path: on line 128 you return if len(parts) < 2, and here you check if it equals 2.

it's reachable. it handles 3 cases: < 2, = 2 and > 2

Now I see the light. It's /(.*?):(.*?)(:o)?/. So either add that as a comment to FromText, or check for parts[2] == 'o' in the else branch to make this explicit.

added check

ineiti · 2024-05-31T09:25:45Z

mino/minows/controller.go

 				"default listen address)",
 			Required: false,
 			Value:    "",
 		},
 	)
+
+	cmd := builder.SetCommand("list")
+	sub := cmd.SetSubCommand("address")


No need to change, only my comment: this could be a command only, w/o a subcommand.

address doesn't work as a command in dvoting. it seems to conflict with commands from another controller when i was testing

it could be addresses, listp2p, listAddresses, dumpAddresses, or any other command.

As I said - no change needed, just a random idea.

ok keeping as-is then :)

ineiti · 2024-05-31T09:35:26Z

mino/minows/rpc.go

+		in:     make(chan Packet, MaxUnreadAllowed),
+	}
+
+	fetch := func(decoder *gob.Decoder) (Packet, mino.Address, error) {


Refactor: add this as a method to orchestrator

are you sure? closure is the more idiomatic way to write single-use functions in Go, as it avoids unnecessarily long argument list through variable capture, which makes the code cleaner, shorter and easier to read without having to scroll to another function.

As far as I can see, there would be no additional arguments, as all captured variables are available in the orchestrator.

And while I can see the argument for one (short) closure, I wouldn't call three closures in a row cleaner, shorter, and easier to read...

ineiti · 2024-05-31T09:35:30Z

mino/minows/rpc.go


-		reply, err := h.Process(mino.Request{Address: from, Message: req})
+	relay := func(packet Packet, dest address) error {


Refactor: add this as a method to orchestrator

ineiti · 2024-05-31T09:35:43Z

mino/minows/rpc.go

-		if err != nil {
-			r.logger.Error().Err(err).Msg("could not reply to call")
-			return
+	receive := func(decoder *gob.Decoder) (Packet, error) {


Refactor: add this as a method to orchestrator

ineiti · 2024-05-31T09:39:13Z

mino/minows/rpc.go

+	encoder := gob.NewEncoder(stream)
+	decoder := gob.NewDecoder(stream)
+
+	receive := func() (Packet, error) {


Refactor: add this as a method to participant (and add decoder to its structure)

ineiti · 2024-05-31T10:21:42Z

mino/minows/session.go

-// Send is asynchronous and returns immediately an error channel that
-// closes when the message has either been sent or errored to each address.
-func (s session) Send(msg serde.Message, addrs ...mino.Address) <-chan error {
+func (o orchestrator) Send(msg serde.Message, addrs ...mino.Address) <-chan error {
 	send := func(addr mino.Address) error {


Make this a method of orchestrator - else it's capturing o here, which is confusing. If it's part of orchestrator, it's clear that it will use the fields of orchestrator

Take msg as parameter, else the captured msg will be used both in this closure and be passed to doSend

ineiti · 2024-05-31T10:22:10Z

mino/minows/session.go

+}
+
+func (p participant) Send(msg serde.Message, addrs ...mino.Address) <-chan error {
+	send := func(addr mino.Address) error {


Make this a method of participant - else it's capturing p here, which is confusing. If it's part of participant, it's clear that it will use the fields of participant

Take msg as parameter, else the captured msg will be used both in this closure and be passed to doSend

XioZ added 27 commits May 9, 2024 18:10

#6 Allows self-dial in call

67d6007

#6 Don't carry source address in packet

a34ca0c

#6 Don't carry source address in packet

5161f56

#6 Allows self-dial for stream using loopback

3a7ce7c

#6 Allows self-dial for stream using loopback

067e69c

#6 Carries source address in packets

9375996

#6 Passes integration_test.go. Shows benchmarks

04d97e0

#6 Passes integration_test.go. Shows benchmarks

6475d7d

#6 Fixing tests

40bc657

#6 Benchmarks

3f15af9

#6 Carries only public multiaddr in packets

89986e9

#6 Uses gob coders

b2c75b8

#6 Uses gob coders

7a0eff4

#6 Don't send to self after session ends. Don't receive message after…

12a70d8

… session ends.

#6 Refactored createSession()

8dc6198

#6 Don't send to self if session ended

8417bc9

#6 Refactored

f29f8d3

#6 Refactored

1fc1146

#6 Listen to end session continuously in a separate goroutine

c7b5001

#6 End session when context closed or stream reset

da38d99

Merge remote-tracking branch 'origin/main' into feature/#9-test-dvoting

326c392

#9 Fixed tests

6a24983

#9 Fixed tests

4b6376e

#9 Print mino address

2605910

#9 Added orchestrator address

0394ed6

#9 Refactored address tests

007c9be

#9 Orchestrator relays messages to destination participants

ac5949b

XioZ assigned XioZ and unassigned XioZ May 30, 2024

XioZ requested a review from ineiti May 30, 2024 23:04

#9 Orchestrator relays messages to destination participants

3901fbe

ineiti requested changes May 31, 2024

View reviewed changes

#9 Extracted closures to methods

2a1cc78

XioZ force-pushed the feature/#9-test-dvoting branch from 227235b to 2a1cc78 Compare May 31, 2024 15:31

ineiti approved these changes Jun 1, 2024

View reviewed changes

XioZ merged commit a745f07 into main Jun 3, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/#9 test dvoting #16

Feature/#9 test dvoting #16

XioZ commented May 30, 2024

ineiti left a comment

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024

ineiti May 31, 2024

XioZ May 31, 2024


		reply, err := h.Process(mino.Request{Address: from, Message: req})
		relay := func(packet Packet, dest address) error {

Feature/#9 test dvoting #16

Feature/#9 test dvoting #16

Conversation

XioZ commented May 30, 2024

ineiti 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

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