Split blob writer code out of larger PR. #685

cody-littley · 2024-08-06T15:07:14Z

Why are these changes needed?

This is code broken off from the larger PR #666

This branch contains the "BlobWriter", which manages the writing of blobs for the traffic generator.

	┌------------┐                                       ┌------------┐
	|   writer   |-┐             ┌------------┐          |   reader   |-┐
	└------------┘ |-┐  -------> |  verifier  | -------> └------------┘ |-┐
	  └------------┘ |           └------------┘            └------------┘ |
	    └------------┘                                       └------------┘

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>

ian-shim · 2024-08-06T19:17:44Z

tools/traffic/test/mock_disperser.go

@@ -0,0 +1,96 @@
+package test


For mocks, you can enforce they adhere to some interface by doing something like this.
Ex. var _ clients.DisperserClient = (*mockDisperserClient)(nil)

Neat trick! I actually noticed a few similar lines in other files and was confused by them.

ian-shim · 2024-08-06T20:01:40Z

tools/traffic/test/blob_writer_test.go

+	var previousData []byte
+
+	for i := 0; i < 100; i++ {
+		if rand.Float64() < errorProbability {


instead of testing the error cases probabilistically, why don't we have an explicit test case that tests the failures?

I changed this to non-random, it now generates an error every tenth cycle: if i%10 == 0. Would you still like to se a stand alone unit test for the error case, or is this sufficient?

ian-shim · 2024-08-06T21:26:50Z

tools/traffic/test/blob_writer_test.go

+
+		// These methods should be called exactly once per tick if there are no errors.
+		// In the presence of errors, nothing should be passed to the unconfirmed key handler.
+		assert.Equal(t, uint(i+1), disperserClient.DisperseCount)


You can also directly match if disperserClient.DisperseBlob method has been called by disperserClient.AssertCalled(...) or disperserClient.AssertNumberOfCalls(...)

I've switched over to your recommended style for mock objects.

ian-shim · 2024-08-06T21:43:05Z

tools/traffic/test/blob_writer_test.go

+		tu.AssertEventuallyTrue(t, func() bool {
+			lock.Lock()
+			defer lock.Unlock()
+			return int(disperserClient.DisperseCount) > i && int(unconfirmedKeyHandler.Count)+errorCount > i
+		}, time.Second)
+
+		// These methods should be called exactly once per tick if there are no errors.
+		// In the presence of errors, nothing should be passed to the unconfirmed key handler.
+		assert.Equal(t, uint(i+1), disperserClient.DisperseCount)


Timing/synchronizing with another goroutine might be tricky just using time. Does this reliably pass? How do you know if L101 will execute after disperser request has been made?
Maybe one thing we could do is make run method public and run test against this method? That way we can assert expectations after the method returns.

Empirically, this was reliably passing (ran it in a loop a few thousand times). Primary reason why I was going through all the trouble of doing it in an attempt to put the tests in another directory without making methods public that shouldn't be public.

I understand your general concern though. I refactored this so that I don't rely on timing. As a result, the test files will end up being in the same package as the other files. Ugly to me, but it seems like a fairly normal practice in go.

Signed-off-by: Cody Littley <[email protected]>

ian-shim

lgtm, left few more comments for you to consider before merging

ian-shim · 2024-08-14T06:09:13Z

tools/traffic/workers/blob_writer.go

+	disperser clients.DisperserClient
+
+	// Unconfirmed keys are sent here.
+	unconfirmedKeyHandler KeyHandler


have you considered passing unconfirmed keys to a channel instead and letting the caller handle it?
I think that's a common pattern in Go

I'm willing to make this change, but since this modifies the API, would it be ok with you if I made this change in the next PR that will add the code that consumes the unconfirmed keys? It will cause some merge conflicts if I change this API in this PR.

ian-shim · 2024-08-14T06:11:03Z

tools/traffic/workers/blob_writer.go

+	data := make([]byte, writer.config.DataSize)
+	_, err := rand.Read(data)
+	if err != nil {
+		panic(fmt.Sprintf("unable to read random data: %s", err))


return an error instead of panicking?

ian-shim · 2024-08-14T06:13:26Z

tools/traffic/workers/blob_writer.go

+	})
+	if err != nil {
+		writer.writeFailureMetric.Increment()
+		writer.logger.Error("failed to send blob request", "err:", err)


nit: no : postfix on labels

ian-shim · 2024-08-14T06:14:39Z

tools/traffic/workers/blob_writer.go

+}
+
+// sendRequest sends a blob to a disperser.
+func (writer *BlobWriter) sendRequest(data []byte) ([]byte /* key */, error) {


you could use a named return values if the returns values look ambiguous
...sendRequest(data []byte) (key []byte, err error)

Interesting, today I learned that syntax exists. :) Change made.

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-08-14T23:43:44Z

tools/traffic/workers/blob_writer.go

+	"fmt"
+	"github.com/Layr-Labs/eigenda/api/clients"
+	"github.com/Layr-Labs/eigenda/encoding/utils/codec"
+	config2 "github.com/Layr-Labs/eigenda/tools/traffic/config"


Is this alias needed? There seems no collision

I blame my IDE, it automatically aliased this when I did an automatic refactor. 🙃

Fixed.

jianoaix · 2024-08-14T23:48:59Z

tools/traffic/workers/key_handler.go

+package workers
+
+// KeyHandler is an interface describing an object that can accept unconfirmed keys.
+type KeyHandler interface {


This is a bit abstract. Is it intended just for unconfirmed blobs? If so, it would be more readable to name these as BlobHandler/AddUncofirmedBlob etc

This will go away in the next PR when I implement a request from @ian-shim to use a channel to pass this data. I'm intentionally not changing it in this PR to reduce the number of merge conflicts in my feature branch.

jianoaix · 2024-08-14T23:49:39Z

tools/traffic/workers/blob_writer.go

+// Start begins the blob writer goroutine.
+func (writer *BlobWriter) Start() {
+	writer.waitGroup.Add(1)
+	ticker := time.NewTicker(writer.config.WriteRequestInterval)


Can this support a write frequency like 0.2 blobs/sec?

Yes. This is the period, aka the time in-between writes. If you set a period of 5 seconds, on average it will write 1/5 or 0.2 blobs per second.

Signed-off-by: Cody Littley <[email protected]>

Split blob writer code out of larger PR.

2268e52

Signed-off-by: Cody Littley <[email protected]>

cody-littley requested review from jianoaix and ian-shim August 6, 2024 15:07

cody-littley self-assigned this Aug 6, 2024

ian-shim reviewed Aug 6, 2024

View reviewed changes

cody-littley added 7 commits August 7, 2024 07:19

Fix test flake.

72b57ba

Signed-off-by: Cody Littley <[email protected]>

Made requested changes.

6c862c2

Signed-off-by: Cody Littley <[email protected]>

More suggested changes.

1508a26

Signed-off-by: Cody Littley <[email protected]>

Made more suggested changes.

cd9da4d

Signed-off-by: Cody Littley <[email protected]>

lint

845477e

Signed-off-by: Cody Littley <[email protected]>

Started refactoring unit test.

94a2c29

Signed-off-by: Cody Littley <[email protected]>

Finsihed refactoring tests.

38288fc

Signed-off-by: Cody Littley <[email protected]>

ian-shim approved these changes Aug 14, 2024

View reviewed changes

Made suggested changes.

656294e

Signed-off-by: Cody Littley <[email protected]>

jianoaix reviewed Aug 14, 2024

View reviewed changes

Made suggested changes.

deea71b

Signed-off-by: Cody Littley <[email protected]>

cody-littley merged commit f07e364 into Layr-Labs:master Aug 16, 2024
6 checks passed

ian-shim pushed a commit to ian-shim/eigenda that referenced this pull request Aug 20, 2024

Split blob writer code out of larger PR. (Layr-Labs#685)

ec77268

Signed-off-by: Cody Littley <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split blob writer code out of larger PR. #685

Split blob writer code out of larger PR. #685

cody-littley commented Aug 6, 2024 •

edited

Loading

ian-shim Aug 6, 2024

cody-littley Aug 8, 2024

ian-shim Aug 6, 2024

cody-littley Aug 8, 2024

ian-shim Aug 6, 2024

cody-littley Aug 13, 2024

ian-shim Aug 6, 2024

cody-littley Aug 8, 2024

ian-shim left a comment

ian-shim Aug 14, 2024

cody-littley Aug 14, 2024

ian-shim Aug 14, 2024

cody-littley Aug 14, 2024

ian-shim Aug 14, 2024

cody-littley Aug 14, 2024

ian-shim Aug 14, 2024

cody-littley Aug 14, 2024

jianoaix Aug 14, 2024

cody-littley Aug 16, 2024

jianoaix Aug 14, 2024

cody-littley Aug 16, 2024

jianoaix Aug 14, 2024

cody-littley Aug 15, 2024

Split blob writer code out of larger PR. #685

Split blob writer code out of larger PR. #685

Conversation

cody-littley commented Aug 6, 2024 • edited Loading

Why are these changes needed?

Checks

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

ian-shim 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

cody-littley commented Aug 6, 2024 •

edited

Loading