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

add witnesses #53

Merged
merged 2 commits into from
Aug 16, 2024
Merged

add witnesses #53

merged 2 commits into from
Aug 16, 2024

Conversation

ryanfkeepers
Copy link
Contributor

witnesses add a targeted producer/consumer system to clues that allow downstream producers to relay data back up to upstream consumer contexts. They're not generally useful, but in certain cases where it becomes difficult to maintain delivery and receipt of clues otherwise they become a sort of necessary evil.

witnesses add a targeted producer/consumer system to clues that allow
downstream producers to relay data back up to upstream consumer
contexts.  They're not generally useful, but in certain cases where it
becomes difficult to maintain delivery and receipt of clues otherwise
they become a sort of necessary evil.
@ryanfkeepers ryanfkeepers added the enhancement New feature or request label Aug 2, 2024
@ryanfkeepers ryanfkeepers requested review from ashmrtn and meain August 2, 2024 21:17
@ryanfkeepers ryanfkeepers self-assigned this Aug 2, 2024
clues.go Outdated
// Relay adds all key-value pairs to the provided witness. The witness will
// record those values to the dataNode in which it was created. All relayed
// values are namespaced to the owning witness.
func Relay(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my favorite name. I'm open to suggestions. Preferably puns about a detective getting clues from a witness.

@ryanfkeepers ryanfkeepers requested a review from pandeyabs August 13, 2024 18:55
Copy link
Contributor

@ashmrtn ashmrtn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Do you have any benchmarks that can help shed light on the potential memory impacts of this? This is mostly just a precaution to make sure that it doesn't cause unexpected memory usage

clues.go Outdated
// witness
// ---------------------------------------------------------------------------

// AddWitness adds a witness with a given name to the contex. The caller can
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: without knowing what a witness is (and having no documentation on an exported Witness type, which itself is fine as it fits into the larger project architecture), having this described as adding a witness to something seems unhelpful

Suggested change
// AddWitness adds a witness with a given name to the contex. The caller can
// AddWitness adds a witness with a given name to the context. The caller can

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check, will update.

datanode.go Outdated
return &dataNode{
parent: dn,
labelCounter: dn.labelCounter,
witnesses: maps.Clone(dn.witnesses),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to test this on different machine architectures. IIRC, I was having trouble with this function returning nil if the input was nil on Mac, which would eventually lead to NPEs when I tried to assign to the map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, thank you

"one": 1,
}, true)

mapEquals(t, ctxWithBob, msa{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you please also make sure that the first two contexts with witnesses aren't changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@ryanfkeepers ryanfkeepers merged commit c6ef710 into main Aug 16, 2024
1 check passed
@ryanfkeepers ryanfkeepers deleted the witness branch August 16, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants