kappa-db / kappa-core

Minimal peer-to-peer database, based on kappa architecture.
254 stars 22 forks source link

WIP: kappa-next #14

Open Frando opened 4 years ago

Frando commented 4 years ago

This PR is the current state of my efforts to make kappa-core work with different kinds of sources. It is based on the #13 and incoroprates a lot of useful feedback from noffle, substack and others.

Breaking changes are:

I added documentation for the new API, see the rendered README.

I think this is ready for review now. If wanted, I can also do a writeup of the changes? But maybe its quite obvious already also from the new README and tests.

Frando commented 4 years ago

Here's an example on what modules using kappa-core would need to change: https://ledger-git.dyne.org/CoBox/kappa-drive/pulls/36/files

I also added a commit that leaves kappa.api[name] available as an alias to kappa.view[name].

m4gpi commented 4 years ago

hey @noffle, cobox plans on supporting @Frando's upgrade to kappa-core, as it provides some pretty important flexibility that we're going to need when managing our indexes. I'd be interested to hear your thoughts on this PR! :)

hackergrrl commented 4 years ago

@Frando is this PR ready for review? I saw WIP in the title so I assumed you're still working on it.

Frando commented 4 years ago

Wow thanks for this review @noffle, cool! I'll try to get to this soon, but might take me a bit more than a week as I'm quite busy with a deadline next week.

jwerle commented 4 years ago

very exciting to watch this!

Frando commented 4 years ago

So finally I'll get back here this week, sorry that it took so long.

I think I'll go over @noffle's review in two steps:

As far as I know Sonar and Cobox are currently depending on this branch, if there are others please chime in too!

Frando commented 4 years ago

I finished a big round of non-api breaking changes from @noffle's review. Yay! I added a couple of comments to things I did not address right away.

Now, to the proposed API changes:

I am using this API in kappa-sparse-indexer and kappa-record-db, which I'd update accordingly then. I think cobox uses only sources internal to kappa-core. @kyphae or did you implement custom sources somewhere? Also if someone else apart from me and @kyphae is using this branch in dependencies, could you give a quick ping here? I'd like to know how "free" I am at the moment with regard to API changes. I've seen @substack using it in examples at least. For those currently depending on it - you can pin to github:Frando/kappa-core#exp-0.2.3 to not get API breaking changes right away if I push to this branch.

ghost commented 4 years ago

I was already expecting some changes so pushing is fine for me.

As far as the argument-prepending goes, I find that I very rarely use this feature because I will almost always do kcore.use(someView(arg1, arg2, ...)) where someView is module.exports = function (arg1, arg2...) {} so where I need the kcore instance or other arguments I pass it in there. Or more commonly, I will pass in particular exported apis instead of the whole kappa-core instance.

hackergrrl commented 4 years ago

:wave:! Hi @Frando, sorry it's taken so long to get back to you on this. It seems like every time there are updates I need to sit down for a long time and download it all into my brain again. :) I appreciate the summary of proposed changes.

The main proposed API change by @noffle is to change the signature of the next callback in a source's pull handler from next({ messages, finished, onindexed }) to next(err, messages, finished, onindexed). And, at the same time, change the default for finished to true. I think I like both changes.

Great!

I have another change I'd propose that is purely asthetic: I'd maybe like to rename ready to sync. It seems clearer to me, as a kappa is never "ready" if it's listening for remote sources that can update at any time. Also it may be confusing as oftenly in our ecosystem ready is an open alias.

Do you think it might be confusing, since it's so similar in meaning to replicate? Could indexesReady() work, or something else that hints at this being a means to wait on the views to catch up? If you disagree, feel free to use sync -- this doesn't feel worth bikeshedding too much on.

One thing I've been thinking about: Currently kappa-core injects itself as a first argument to the API functions. I added a context op to make this overridable with another object, as oftenly I've seen me needing the "object in which the kappa is used" (e.g. a cabal, a kappa-record-db) and not the kappa-core itself. At the same time, this can also be passed as an option into the createView or createSource function. So I thought if we should just remove this injected first argument to the API functions. Thoughts?

I'm down with this change. I really regret the kludge I chose of injecting fake arguments.

The internal state for each flow currently is either Ready, Running, Paused. Likely we want to have an Error state also, into which a flow is put when either a view returned an error from map or a source returned an error from pull

Yes please.

Maybe we want to expose a getState function that returns the current state of things, and also asks the source for the indexing state (as in the recently added feature to multifeed-index)

I'm ok with punting on this until after these changes get merged. This PR is also very big, so anything we can do to keep the scope down and change later would be :fire:.