Streaming hoard #84

silasdavis · 2019-11-28T17:20:36Z

No description provided.

Signed-off-by: Silas Davis <silas@monax.io>

- Remove use of oneof - note this should be backwards compatible - Update to version 7 and write changelog - Code cleanup and refactor Signed-off-by: Silas Davis <silas@monax.io>

gregdhill

Before tagging we need to do a version bump on all imports and in the go mod file. Without oneof, I question why we need to enforce message ordering server side. In the case of the salt (which is just another byte slice) why can't we simply append? Otherwise this looks good! Might be worth adding some documentation on using the js lib, but that is low priority.

streaming.go

gregdhill · 2019-12-03T10:12:39Z

streaming.go

+			return fmt.Errorf("recevied multiple salts but there can be at most one")
+		}
+		if len(accum.Data) > 0 {
+			return fmt.Errorf("recevied salt after data but salt must come before all data chunks")


Suggested change

return fmt.Errorf("recevied salt after data but salt must come before all data chunks")

return fmt.Errorf("received salt after data but salt must come before all data chunks")

gregdhill · 2019-12-03T10:14:03Z

streaming.go

+	if chunk == nil {
+		return nil
+	}
+	if len(chunk.Salt) > 0 {


Why do we need to enforce these constraints on the salt?

gregdhill · 2019-12-03T10:15:54Z

document.go

@@ -7,12 +7,12 @@ import (
 	"fmt"

 	"github.com/gogo/protobuf/proto"
+	"github.com/monax/hoard/v6/api"


I think all hoard imports should point to v7 now...

gregdhill · 2019-12-03T10:16:46Z

go.mod

@@ -2,32 +2,28 @@ module github.com/monax/hoard/v6



This should point to v7.

gregdhill · 2019-12-03T10:18:40Z

streaming.go

+	"fmt"
+	"io"
+
+	"github.com/monax/hoard/v6/api"


gregdhill · 2019-12-03T10:19:35Z

streaming.go

+
+		if d.Meta != nil {
+			if accum.Meta != nil {
+				return nil, fmt.Errorf("recevied multiple document meta but there can be at most one")


Suggested change

return nil, fmt.Errorf("recevied multiple document meta but there can be at most one")

return nil, fmt.Errorf("received multiple document meta but there can be at most one")

Why can we only consume one?

If we add two - which one is the real one?

Or are you consider we allow them to be composited - this seems like an unnecessary complication. Almost by definition we don't expect our metadata to be so large - much better if we can be sure once we have read the metadata we have all of it.

streaming.go

gregdhill · 2019-12-03T10:20:46Z

streaming.go

+			return fmt.Errorf("recevied multiple grant specs but there can be at most one")
+		}
+		if len(accum.Plaintext.Data) > 0 {
+			return fmt.Errorf("recevied grant spec after data but spec must come before all data chunks")


Suggested change

return fmt.Errorf("recevied grant spec after data but spec must come before all data chunks")

return fmt.Errorf("received grant spec after data but spec must come before all data chunks")

gregdhill · 2019-12-03T10:20:56Z

streaming_test.go

+	"io"
+	"testing"
+
+	"github.com/monax/hoard/v6/api"


silasdavis · 2019-12-03T10:40:04Z

Without oneof, I question why we need to enforce message ordering server side. In the case of the salt (which is just another byte slice) why can't we simply append?

The primary reason is that we want to be able to read the beginning of a stream sequentially and get the metadata without reading further in. If we allow out-of-order transmission then either we need to re-order it which is a pain or we loose the ability to take the HEAD of a stream and extract the metadata.

We could append on the salt but again we provide flexibility but there is no good reason to split up a salt - it is probably a mistake.

Generally having stronger invariants is better than have more arbitrary flexibility if it is not clear what good that does. We basically have some descriptive metadata and some stream. If it wasn't for the way plaintexts are handled I'd be tempted to have a message like:

message Document {
   Metadata Data = 1;
   bytes Data = 2;
}

And include the salt, metadata, grantspec and whatever other stuff you are only meant to say once in there. We could still go this route but I didn't want to make too many changes.

Might be worth adding some documentation on using the js lib, but that is low priority.
I have put a far few comments in the code - I'm not sure I want to maintain more detached documentation at this point...

Signed-off-by: Silas Davis <silas@monax.io>

Update deps and build with commit hash

a0f05b2

Signed-off-by: Silas Davis <silas@monax.io>

silasdavis force-pushed the streaming-hoard branch 9 times, most recently from e4d75b0 to 40282c5 Compare December 2, 2019 11:41

silasdavis changed the title ~~WIP: Streaming hoard~~ Streaming hoard Dec 2, 2019

silasdavis force-pushed the streaming-hoard branch 7 times, most recently from 52a8d8f to 87a97b9 Compare December 3, 2019 09:17

Implement streaming client for hoard-js

22ce590

- Remove use of oneof - note this should be backwards compatible - Update to version 7 and write changelog - Code cleanup and refactor Signed-off-by: Silas Davis <silas@monax.io>

silasdavis force-pushed the streaming-hoard branch from 87a97b9 to 22ce590 Compare December 3, 2019 10:03

gregdhill suggested changes Dec 3, 2019

View reviewed changes

Update imports to v7

dcb4490

Signed-off-by: Silas Davis <silas@monax.io>

silasdavis force-pushed the streaming-hoard branch from b8cdf94 to dcb4490 Compare December 3, 2019 10:40

gregdhill merged commit 92dad7c into monax:master Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streaming hoard #84

Streaming hoard #84

silasdavis commented Nov 28, 2019

gregdhill left a comment

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

silasdavis Dec 3, 2019

gregdhill Dec 3, 2019

gregdhill Dec 3, 2019

silasdavis commented Dec 3, 2019

	return fmt.Errorf("recevied salt after data but salt must come before all data chunks")
	return fmt.Errorf("received salt after data but salt must come before all data chunks")

	return nil, fmt.Errorf("recevied multiple document meta but there can be at most one")
	return nil, fmt.Errorf("received multiple document meta but there can be at most one")

	return fmt.Errorf("recevied grant spec after data but spec must come before all data chunks")
	return fmt.Errorf("received grant spec after data but spec must come before all data chunks")

Streaming hoard #84

Streaming hoard #84

Conversation

silasdavis commented Nov 28, 2019

gregdhill 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

silasdavis commented Dec 3, 2019