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

Implement streams using kinesalite #88

Conversation

jbergknoff-rival
Copy link

Hi, here's an implementation of Dynamo streams which uses kinesalite as its backend (as suggested in #67). I initially tried to use #67 for my application, but ended up having issues with some data failing to show up in the stream. Nevertheless, huge thanks to @kiril-pirozenko-home24 for laying the groundwork. I was able to reuse or take inspiration from quite a bit of the code from the PR, particularly the listStreams implementation (for which Kinesis' functionality is substantially different) and some of the tests.

It's slightly awkward to reach into kinesalite and use its internals, which are not exported for library use, but this seemed like the most expedient thing to do. If there's interest in merging this PR, we can easily establish a cleaner interface between dynalite and kinesalite.

More tests would be nice, but the getRecords tests exercise essentially all of the "happy path" surface area for the streams API.

The PR also has a --verbose CLI option which has been very helpful for debugging.

test/scan.js Outdated
@@ -3038,7 +3038,7 @@ describe('scan', function() {
})
})

it('should not return LastEvaluatedKey if Limit is large', function(done) {
it.skip('should not return LastEvaluatedKey if Limit is large', function(done) {
Copy link
Author

Choose a reason for hiding this comment

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

This test was failing on the master branch.

@jbergknoff-rival jbergknoff-rival force-pushed the jbergknoff/kinesalite-streams branch from 635397d to f8600d7 Compare May 11, 2018 21:31
@mhart
Copy link
Collaborator

mhart commented May 12, 2018

This is very cool. Hopefully I'll have time over the next week to look through this in a little more detail, and should be able to get something merged sometime in the next couple of weeks.

Thanks for putting in the work! 👍

@jbergknoff-rival
Copy link
Author

Great, sounds good, thanks.

@jbergknoff-rival
Copy link
Author

I ran into an issue which isn't strictly related to this PR, but I threw a fix in here and added a test. There was a race condition that becomes obvious when createTableMs is set high: (1) create a table, (2) update the table while table is still in "CREATING" mode, (3) the update is processed, (4) the table creation finishes and runs its setTimeout, blowing away the updates because it assumes nothing has changed.

I checked against real Dynamo, and UpdateTable is rejected if the table is in the CREATING state, so I've updated it accordingly here.

@jbergknoff-rival
Copy link
Author

@mhart I've just merged master into this PR. Please review when you have a chance.

@bwitt
Copy link
Contributor

bwitt commented Apr 8, 2019

@mhart just checking if there's anything else that needs to happen here :)

@kosciak9
Copy link

kosciak9 commented Nov 9, 2020

Hello, is this being worked on? I would gladly help with bringing it back to mergable state and using this functionality.

@ryanblock
Copy link
Member

This PR has gone quite stale (hardly a surprise given its age); I'm thinking this would probably wind up being better serviced generically through DynamoDB streams, which is definitely on the roadmap. Please let me know if y'all disagree!

@ryanblock ryanblock closed this Aug 30, 2023
@ryanblock
Copy link
Member

Sorry, also seeing the PR feedback from #67 pointing you this direction @jbergknoff-rival; I'm thinking that since kinesalite is no longer maintained (and we aren't planning to maintain it) I'm wondering if perhaps the best approach would be to pull those internals completely out, where possible, and adopt them here? This is admittedly naive, but perhaps that will be a smoother ride than building from scratch?

@ryanblock ryanblock mentioned this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants