Add TTL support #144
Add TTL support #144
Conversation
exoego
force-pushed
the
ttl-support
branch
2 times, most recently
from
9853879
to
a0dae69
Compare
|
I think This PR is now ready to be reviewd. Also, I recommend to setup GitHub Actions via #146 |
db/index.js
Outdated
| }) | ||
| }) | ||
| }) | ||
| }, 1000) |
There was a problem hiding this comment.
I wonder how long the expiry job may run if there are many tables or large tables. 🤔
I occasionally dump a lot of data into dynalite in order to exercise business logic code, in which case, table scan can easily take longer than 1s.
Btw., aws dynamodb expiry is not guaranteed to happen on the second, in fact it can be delayed by hours even a day.
There was a problem hiding this comment.
Btw., aws dynamodb expiry is not guaranteed to happen on the second, in fact it can be delayed by hours even a day.
Yeah, that is true.
According to the docs, "TTL typically deletes expired items within 48 hours of expiration".
I am considering to make it configurable.
Currently, I set it 1000 to minimize test time.
table scan can easily take longer than 1s.
I am also considering to optimize table scan.
There was a problem hiding this comment.
Per my comments below, I do still share overall concerns about efficiency – moreso seeing that folks like @dimaqq dump large amounts of data into this thing. I def think this can get figured out though!
|
Any chance this is merged soon? |
|
|
db/index.js
Outdated
| var currentUnixSeconds = Math.round(Date.now() / 1000) | ||
|
|
||
| function logError(err, result) { | ||
| if (err) console.error("@@@", err) |
There was a problem hiding this comment.
idk about @@@ but we should plug this into #10 (or whatever our verbose logging is, when ready).
db/index.js
Outdated
| }, {}) | ||
| var data = {TableName: name, Key: itemKey} | ||
| var cb = function (err) { | ||
| // Noop ? |
| if (err) console.error("@@@", err) | ||
| } | ||
|
|
||
| lazyStream(tableDb.createKeyStream({}), logError) |
There was a problem hiding this comment.
This may be a completely ignorant comment but is it possible this is an inefficient way to query our table names?
Overall I'm very curious as to how expensive this operation is to run. It's probably not likely to blow up a modern machine, but it's O(n^lots). I wonder if a more efficient approach would be to create an internal ttl index and query that instead of streaming out + operating on the contents of each table.
There was a problem hiding this comment.
I do not have time to create such an index for efficient traversing.
test/updateTimeToLive.js
Outdated
| }) | ||
| }) | ||
| }) | ||
| }, 3000) |
There was a problem hiding this comment.
Possible this could be reliably reduced? Those seconds add up in test suites!
There was a problem hiding this comment.
Replaced the fixed setTimeout(..., 3000) with setInterval(... 50) in 669eea4
On my machine, the test ends somewhere between 1900-2000 ms.
|
Hi, it looks to me that everything is ready to be merged. May I ask when will all the commits be merged? |
|
@tianshu123 we'll take next steps once I've heard back from @exoego. I haven't seen any remarks or resolved conversations. |
Closes #103
setIntervalto support legacy Node.js not having Worker threadsTODO
UpdateTimeToLive