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

Contributing a feature - is it relevant? #20

Closed
shaiker opened this issue Jan 8, 2018 · 3 comments
Closed

Contributing a feature - is it relevant? #20

shaiker opened this issue Jan 8, 2018 · 3 comments

Comments

@shaiker
Copy link

shaiker commented Jan 8, 2018

Hey there,
I'm using your TS module, and i encountered a use case in which i need to:

  1. update existing value for given timestamp
  2. add values for history (i.e. with timestamps that are older than existing ones)

I'm thinking about developing those features, and opening a pull request, But i was wondering if it is at all relevant, and will you be willing to merge and add those features in to the project?
(of course assuming my code will be decent :-) )

@danni-m
Copy link
Owner

danni-m commented Jan 8, 2018

First of all, contributing is always welcomed.

Regarding the features:

  1. This is supported at the moment for the latest timestamp only, Im planning of adding compression to the data, once we have compression its harder to update existing timestamp without a big overhead.
    having said that, with the simple chunk interface we can support updating without a problem, I would be willing to accept a PR for that as long as it kept in the chunk level.
  2. back filling is a problem, currently the chunk design is implmented in a way that adding a timestamp in a middle of a chunk is a not easy. you would need to move data around and possibly create a new chunk.
    I can see an easy implementation if the timestamps you are backfilling are older then the oldest timestamp at the moment.

Another possible implementation for this would be done in the client side where you copy all the data from a key and re-insert it to another key.

@shaiker
Copy link
Author

shaiker commented Jan 8, 2018

I made a PR for (1) - #21

regarding (2) - I guess I'll do it on the client as you suggested :-)

Cheers.

P.S.
Just out of curiosity - Perhaps I'm just not familiar enough with how Redis manages modules, but I noticed that in ChunkItertorGetNext and SeriesItertorGetNext you're doing memcpy and creating a copy of a Sample, but never freeing that copy's memory. Doesn't it create memory leaks ?

@danni-m
Copy link
Owner

danni-m commented Jan 10, 2018

Regarding the memory, memcpy used for copying data around, it doesn't allocate more memory.
in SeriesItertorGetNext we create a variable internalSample on the stack, this variable location is passed to SeriesItertorGetNext, when we exit the function this variable will be deleted from the stack.

Generally, the caller to ChunkItertorGetNext and SeriesItertorGetNext are incharge of allocating and releasing the memory.

@shaiker shaiker closed this as completed Feb 1, 2018
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

No branches or pull requests

2 participants