docs: Document core public stuff #40

AndrewSisley · 2025-01-24T20:24:59Z

Relevant issue(s)

Resolves #30

Description

Documents the core public types/funcs/etc. Also removes a couple of unused items.

I'd like to use this PR as an excuse to review the interfaces/behaviours - please don't just review the quality of the new documentation, please review whether you agree with the thing that it is documenting.

I've added a couple of discussion items as PR comments, please continue those discussions, and add your own to anywhere you see fit.

I'll add issues for any changes we agree to make before merging this PR.

kv.go

AndrewSisley · 2025-01-24T20:37:48Z

kv.go

+	End []byte
+
+	// Reverse the direction of the iteration, returning items in
+	// lexographically descending order of their keys.
 	Reverse bool


discussion: I think the Prefix, Start, End and Reverse options provide a large space for misunderstanding from both a consumer and maintainer perspective.

I think we can remove the ability for #32 to occur, as well as make the interplay of Start/End with Reverse clearer - by removing the ability for consumers to have certain combinations.

I think following structure would be less ambiguous:

type PrefixOptions struct { Prefix []byte Reverse bool } type IterOptions struct { // Reverse is handled by `From` having a larger value that `To` From []byte // From is always inclusive To []byte // To is always exclusive PrefixOptions PrefixOptions }

We could also use some interface/type magic to prevent consumers from specifying both a Prefix and a Start and/or End, but I think that would result in an ugly struct and make things harder/less-clear for consumers.

No strong opnions here but I went though the same issues while thinking about Raccoons KV and Iterator interfaces (it's currently slightly different from corekv because Raccoon Iterators are composable and generic).

But I settled on not having Prefix iteration as an option for Iteration, wrapping a KV Store in a Prefix Store is a one liner and a separate method can handle prefix iteration altogether

Chatting with Bruno on discord made me realise Iterator could just extend the Enumerable interface, then we would get an awful lot for free, and it could save him a fair amount of code on the Raccoon side of things.

I don't have too much of a preference regarding the distinction between range (start/end) and prefix iterators. I do think the separation could be nicer/clearer.

As for the Enumerable idea, I can't say I'm a fan. The intent/hope for corekv is to not be too opinionated about the downstream use of the kv store, and instead just provide a base set of interfaces/options to get working and consistent across projects. The typed/generic nature of the Enumerable and the goals of Raccoons specific marshaling/composition is much more narrow.

I think we can potentially create a separate sub package within corekv that takes in as a parameter the type specific encoding/decoding stuff, and produces a typed Enumerable. Similar to the namespace package in design, but I wouldn't want the decisions of that package/use case to influence just the base level utility of the Store and Iterator interfaces.

I'm also not a big fan of Enumerable for this relatively low level of abstraction.

No strong preferences from my side as well. Couple other options to consider:

expose NewPrefixOptions and NewRangeOptions. We can also make IterOptions a private struct, but that feels a bit weird.

Instead of Iterator method have RangeIterator and PrefixIterator

What do you guys not like about the Enumerable interface? I don't think I understand your objections to it.

If we change the iterator interface like this, it would satisfy the Enumerable interface:

type Iterator interface { // Next() old, return values changed (seek should do the same to match, but that is not required for Enumerable Next() (bool, error) // new, wanted by me anyway due to the perceived problems with `Valid` Reset() // new, resets the iterator, handy in it's own right }

Then the all CoreKV iterators will work with enumerable.Where, enumerable.Select, etc, etc with zero extra development.

and 2. Instead of Iterator method have RangeIterator and PrefixIterator

I like this thought, thanks Islam :)

I thought you wanted to use the enumerable package in here. Just making it compatible with enumerable is probably fine.

I thought you wanted to use the enumerable package in here. Just making it compatible with enumerable is probably fine.

I would embed the interface within the Iterator interface, as I see it's implementation as a feature worthy of compile-time protection and documentation, but Enumerable is just an interface.

Discussed in standup, we will move to implementing Enumerable.

Fred is pretty against embedding it directly, and the compile safety/description we gain from it might not be worth the prod-code dependency.

I will strongly consider relying on comment-documentation and test code-only to enforce interface parity here.

I will not expose the generic (yet :)).

AndrewSisley · 2025-01-24T20:40:52Z

kv.go

 type Iterator interface {
+	// The behaviour of Domain is undefined and varies heavily depending on
+	// which store implementation is being iterated over:
+	// https://github.com/sourcenetwork/corekv/issues/31
 	Domain() (start []byte, end []byte)


discussion: I think this should be removed. It's behaviour is unclear, and I see the return type as flawed, as it is trying to massage a Prefix into a start/end.

We could potentially replace it with a Options() IterOptions func, but I am currently in favour of not bothering with that for now and just delete it.

I agree with this sentiment, the Domain method came from inspiration from another package, but its utility is a little unclear.

I might need to double check if corekv is used within the SourceHub side of things does the lack of Domain cause problems there. Since the method comes from the cosmos-db interface (if my memory is correct).

If it's not needed, I agree that it should be removed.

kv.go

AndrewSisley · 2025-01-24T20:52:10Z

kv.go

 type TxnStore interface {
 	Store
+
+	// NewTxn returns a new transaction.
 	NewTxn(readonly bool) Txn


discussion: Given that we have read-only types, I am in favour of splitting this function into two:

type TxnStore interface { NewTxn() Txn NewReadOnlyTxn() Reader }

discussion: Given that we have read-only types, I am in favour of splitting this function into two:

Not sure what you mean by "read-only types"?

As for the actual proposed changes. Im against this. I don't think it adds enough to really make a difference. The consumers can compose their own interfaces (like we do in Defra) that are read specific, and assign the Txn to that interface, all in one line. Which gives them the same amount of type safety. This is specifically why the interfaces for corekv are already broken up into Reader and Writer.

I'm going to park this for now, as this and this feel like a long-running clash of styles between myself and John, where I favour slightly more complex interfaces in return for extra type safety. Neither this change, or the one linked, should not become notably more difficult by postponing if we later chose to make the change(s).

AndrewSisley · 2025-01-24T21:59:14Z

kv.go

+	// https://github.com/sourcenetwork/corekv/issues/34
+	//
+	// It is very likely ignored for the memory store iteration:
+	// https://github.com/sourcenetwork/corekv/issues/33
 	KeysOnly bool


discussion: We could remove this option, replacing it with a keys-only iterator func, that lacks Value related stuff:

type KeyIterator interface { // e.g. no `Value` function } type Store interface { Iterator() Iterator KeyIterator() KeyIterator }

Preference to keep this as is, keeping the Store interface simpler (read: less methods) is a priority imo. I don't think we win much in practice for the type safety of a KeyIterator with no Value method, at the cost of maintaining a larger interface that all stores need to implement separately, vs just tracking the value in the options struct and omitting the value.

It is very likely ignored for the memory store iteration:
It seems like the referenced bug is partly the inspiration for wanting this separation of interfaces. But I think that its trying to be too clever/complex with the type system to fix what is a relatively basic bug. Yes the bug could never exist in the first place if we had these proposed interfaces, but I don't that warrants this suggestion.

I'm going to park this for now, as this and this feel like a long-running clash of styles between myself and John, where I favour slightly more complex interfaces in return for extra type safety. Neither this change, or the one linked, should not become notably more difficult by postponing if we later chose to make the change(s).

jsimnz · 2025-01-25T00:06:54Z

praise: I really like the format/approach of this PR. Using a documentation PR to highlight specific semantics/design choices, and proposing new ones at the same time as documenting, very nice. Really lets us just focus on the merits of the different options, rather than the details of the implementation. 👍

kv.go

islamaliev · 2025-01-25T08:00:07Z

kv.go

-	// Range iteration from start (inclusive) to end explusive.
-	// Start must be lexigraphically less
-	// then end, unless `nil` is used.
+	// If Prefix is nil, and Start is provided, the iterator will


suggestion: what if Prefix is not nil, but empty? Would be nice to have it cover in docs

I see documenting nil or empty as bit unnecessary here and don't really bother - I'm curious as to whether this is a minority view here though now 😁? @sourcenetwork/database-team

The check is on a length and a nil byte slice has a length of 0. It's the same behaviour. Not sure if we need to say anything specific about it. We could just say If Prefix is nil or empty....

islamaliev · 2025-01-25T08:21:17Z

kv.go

+	End []byte
+
+	// Reverse the direction of the iteration, returning items in
+	// lexographically descending order of their keys.
 	Reverse bool


I'm also not a big fan of Enumerable for this relatively low level of abstraction.

No strong preferences from my side as well. Couple other options to consider:

expose NewPrefixOptions and NewRangeOptions. We can also make IterOptions a private struct, but that feels a bit weird.

Instead of Iterator method have RangeIterator and PrefixIterator

kv.go

fredcarle · 2025-01-27T16:08:35Z

kv.go

+	// If Prefix is nil, and Start is provided, the iterator will
+	// only yield items with a key lexographically smaller than this
+	// value.
+	//
+	// Providing an End value equal to or smaller than Start
+	// will result in undefined behaviour:
+	// https://github.com/sourcenetwork/corekv/issues/32
+	End []byte


todo: This is the same documentation as Start. It should be specific to End

Nearly the same, nice spot lol - I flipped greater than or equal to smaller but forgot to swap Start for End in that first sentence 😁

I think last sentence can be the same for both.

Fix copy-paste problem

fredcarle · 2025-01-27T16:32:07Z

kv.go

 	Iterator(ctx context.Context, opts IterOptions) Iterator
 }

+// Writer contains functions for mutating stuff stored within a store.


suggestion: Mutating stuff -> Mutating values

I do disagree, as it is not just for mutating values, but anything in the store (e.g. adding/removing keys). 'Stuff' was a deliberate choice on my part.

There are only two things stored in our stores and you can't have one without the other. They are key and value. So if you delete a value you are effectively deleting the kv pair. You are not just adding a key, you are setting a kv pair. It's not stuff. We can be more precise here. I don't see the reason for the ambiguity.

Ah I see what you mean, and the lack of a Move-like func on the interface supports this thought atm, and transactions/iterators (tracked/mutated in store state) are not impacted either by this particular interface. I'll use the word value.

Mutating stuff -> Mutating values

kv.go

fredcarle · 2025-01-27T16:52:16Z

kv.go

+// Txn hides changes made to the underlying store from this object,
+// and hides changes made via this object from the underlying store
+// until `Commit` is called.


suggestions: I don't like using hides to describe a transaction. I would tend to say that changes in a transaction are made in isolation of the underlying store until committed.

Will look at rewording, I didn't like 'hides' either, but struggled writing this comment.

Reword

If anything ever implements them, the interfaces are quite easy to add back in.

If the prefix is nil, and the start/end is nil, the iterator will iterate over the entire store, it does not matter whether or not internally it uses a 'prefix' or 'range' iterator.

jsimnz

Approving now to unblock you. I think all the important points have been raised in their respective comment threads. Not sure if there are any outstanding, but afaik your progressing through them. So once its feeling good for you, send it 👍.

Feel free to ping on discord if there are any specific remaining threads you feel still need input

AndrewSisley · 2025-01-29T05:37:56Z

Approving now to unblock you. I think all the important points have been raised in their respective comment threads. Not sure if there are any outstanding, but afaik your progressing through them. So once its feeling good for you, send it 👍.

Thanks John! Yeah, the main disruptive points have all been resolved I think, the others can still be discussed if/when people want but I am happy to park them for now - they won't be too painful to make after integrating CoreKV into Defra, and I don't want to spend forever on this :)

I'll probably merge this in the morning. Conversations can be continued in this PR post-merge if people want.

Thanks for all your input here :)

AndrewSisley added the documentation Improvements or additions to documentation label Jan 24, 2025

AndrewSisley added this to the CoreKV v0.1 milestone Jan 24, 2025

AndrewSisley self-assigned this Jan 24, 2025

AndrewSisley commented Jan 24, 2025

View reviewed changes

kv.go Show resolved Hide resolved

AndrewSisley commented Jan 24, 2025

View reviewed changes

kv.go Show resolved Hide resolved

AndrewSisley commented Jan 24, 2025

View reviewed changes

AndrewSisley requested review from Lodek and a team January 24, 2025 20:53

AndrewSisley commented Jan 24, 2025

View reviewed changes

AndrewSisley force-pushed the 30-doc-pub-types branch from 54a6e1c to 58fd98d Compare January 24, 2025 22:22

islamaliev reviewed Jan 25, 2025

View reviewed changes

fredcarle reviewed Jan 27, 2025

View reviewed changes

kv.go Show resolved Hide resolved

fredcarle reviewed Jan 27, 2025

View reviewed changes

AndrewSisley requested review from fredcarle, islamaliev and jsimnz January 28, 2025 18:32

AndrewSisley added 9 commits January 28, 2025 13:46

Remove unused batch interfaces

58ec957

If anything ever implements them, the interfaces are quite easy to add back in.

Remove unused Item interface

59b615b

Remove unhelpful implementation detail

ae14588

If the prefix is nil, and the start/end is nil, the iterator will iterate over the entire store, it does not matter whether or not internally it uses a 'prefix' or 'range' iterator.

Update existing documentation for IterOptions

f1d09da

Document interfaces

d65db19

PR FIXUP - Expand Seek documentation

882cf73

PR FIXUP - Fix start vs end typo

3167dfc

PR FIXUP - Change stuff to values

17ebf83

PR FIXUP - Hides changed to Isolates

5296a15

AndrewSisley force-pushed the 30-doc-pub-types branch from acd39c2 to 5296a15 Compare January 28, 2025 18:46

jsimnz approved these changes Jan 29, 2025

View reviewed changes

AndrewSisley merged commit 89a74e7 into sourcenetwork:main Jan 29, 2025
4 checks passed

AndrewSisley deleted the 30-doc-pub-types branch January 29, 2025 17:46

AndrewSisley mentioned this pull request Jan 30, 2025

fix: Remove Iterator.Domain #46

Merged

docs: Document core public stuff #40

docs: Document core public stuff #40

Conversation

AndrewSisley commented Jan 24, 2025

Relevant issue(s)

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jan 27, 2025 • edited Loading

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

AndrewSisley Jan 24, 2025 • edited Loading

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

jsimnz commented Jan 25, 2025

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

AndrewSisley Jan 27, 2025 • edited Loading

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

AndrewSisley Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

AndrewSisley commented Jan 29, 2025

AndrewSisley Jan 24, 2025 •

edited

Loading

AndrewSisley Jan 27, 2025 •

edited

Loading

AndrewSisley Jan 27, 2025 •

edited

Loading

AndrewSisley Jan 24, 2025 •

edited

Loading

AndrewSisley Jan 27, 2025 •

edited

Loading

AndrewSisley Jan 27, 2025 •

edited

Loading

AndrewSisley Jan 27, 2025 •

edited

Loading