Skip to content
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

Update set_digest to reflect if the key was stored in the same node #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NavyaZaveri
Copy link

@NavyaZaveri NavyaZaveri commented Mar 14, 2021

I think there's a minor issue on the networking layer: if a given node storse the key in itself, but all calls to its neighbors failed, set would return still False -- which would be incorrect.

@bmuller
Copy link
Owner

bmuller commented Mar 15, 2021

Thanks for taking a look at this @NavyaZaveri! I think the purpose of the boolean response is to indicate whether the value was set on the network, irrespective of whether the current node may have stored it as well. If all calls to the network failed (for instance, this node can no longer reach any other nodes) then I think the intended response should be False.

@NavyaZaveri
Copy link
Author

NavyaZaveri commented Mar 16, 2021

I think the purpose of the boolean response is to indicate whether the value was set on the network, irrespective of whether the current node may have stored it as well.

Ok, I see! I'm not sure I understand why that's the case though. Is that a detail of the protocol? As long as it's set somewhere on the network, as a user, it makes sense to receive a True from set, reflecting that the pair has been set onto the network (which includes the current node). When it returns False, I would imagine it didn't manage to set the kV pair anywhere on the network because of node failures or the expected nature of udp.

@bmuller
Copy link
Owner

bmuller commented Mar 16, 2021

If the node has no neighbors, then False is returned whether or not the current node could store the value. That behavior is an example of what the function result indicates - "was the value successfully set on the network". Simply storing locally doesn't satisfy that question, especially in a case when the network isn't reachable.

The point of the return value is to indicate whether or not there was a durable set that occurred. If the entire network is down, but the local node stored the value, then it wasn't stored in a durable way (if the current node goes down, no one else will have the value).

@NavyaZaveri
Copy link
Author

NavyaZaveri commented Mar 16, 2021

Ok, that makes sense, thanks! :)

Do you think it's worth explicitly defining what durability means on a code-level? How many neighbors does it have to be set into for it to be considered durable?

The point of the return value is to indicate whether or not there was a durable set that occurred. If the entire network is down, but the local node stored the value, then it wasn't stored in a durable way (if the current node goes down, no one else will have the value).

Based on this explanation, I think having a concrete number for durability might make sense. For example, the kv pair is stored in exactly one neighbor, then the function would still return True, directly contracting the intended semantics.

@bmuller
Copy link
Owner

bmuller commented Mar 16, 2021

Sure, I think it would be a great idea to update the function docs to indicate which conditions would lead to a True vs False result. For the purposes of those docs, being able to store on at least one other node on the network would be considered sufficient for a True result.

I'd be happy to merge an updated PR if you want to just update the function docs for set and set_digest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants