Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement v2 client GET functionality #972
base: master
Are you sure you want to change the base?
Implement v2 client GET functionality #972
Changes from 4 commits
4625e96
f07e820
885c131
6848663
a48afb1
225f2a3
d265f6a
e9d91c5
dd3c262
88df865
505a1f0
2b87633
cf1cd80
53893d8
0373dd7
826a026
975b6e5
0666d24
0a49aa5
1193ce7
496e277
2d392ff
db51291
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting this thread to discuss package structure. I personally don't like that all our clients are in the same package. I think I would prefer if each client was in its own separate package, because that allows to have cleaner names, and make sure to prevent clashes. For eg:
is way cleaner than having long names all under clients package.
I'm fine if people prefer to keep all the clients in same namespace, but at least I think we should separate the v1 and v2 namespace. Thoughts? @ian-shim @epociask @bxue-l2 @jianoaix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. We are getting a lot "v2" in the names around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope for this PR but curious if we'd ever wanna let users define their own retrieval policies when communicating with relays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly something to consider, at the latest if/when users have to pay relays for retrieval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why this choice. How is the relayKey list populated? Shouldnt we let the person populating be able to dictate some preference on the relays by iterating through them in normal order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My logic (which may be based on incorrect assumptions) is that, since the relayKey list is part of the cert, the order would the same for every client.
It seems we wouldn't want all clients to attempt the first relay in the list, rather we should distribute load across all available relays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I think this makes sense. Although now I just realized I'm confused myself. @ian-shim does this make sense? Are the relayKeys meant to be for data replication or sharding? Aka do we need to hit all the relayKeys to get all the blobs, or any ONE should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The disperser implementation itself shuffles the relay keys, so attempting each relay in order is actually fine. But it's good that we're not assuming that relay keys aren't ordered in any particular way.
Any one should do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ian-shim The randomization is done while creating the cert, and all clients will be given the same cert from the disperser, right?
So, for a given blob that can be served by relays
[X, Y, Z]
, if all clients were to attempt the relays in order, then they all would try to get the blob from relayX
, and relays[Y, Z]
wouldn't receive any load.Is this the correct understanding, and if so, are you saying this behavior would be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the circumstance where the error is transient and the # of relay keys == 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that we have an additional timeout, during which the client repeatedly retries all relays?
I could implement this if it's the way we want to go- but I don't see how the case
relay keys == 1
is unique?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To expand these invariants: an honest relay should never send a blob which doesn't respect its polynomial commitments. The thing is though this check would get caught upstream (i.e, within proxy directly) and probably cause the request to fail. The proxy client would trigger a retry which would probably route to another relay.
this isn't a big problem rn and we can just document it somewhere for circle back sometime in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to check this invariant here, included in this PR? Seems like it wouldn't be hard to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this getter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I carried it forward from v1 client -
will remove as it may not be necessary for v2(nevermind, see comment from Sam below)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll still need it. We use it in proxy to get the codec and IFFT blobs when computing commitments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knit^2: disagree here, been trying to enforce https://github.com/uber-go/guide/blob/master/style.md#error-wrapping