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

Trim Old Blocks #100

Merged
merged 2 commits into from
Oct 19, 2014
Merged

Trim Old Blocks #100

merged 2 commits into from
Oct 19, 2014

Conversation

BooDoo
Copy link
Contributor

@BooDoo BooDoo commented Oct 12, 2014

Addresses #59

When a new BlockBatch is fetched in updateBlocks, trim BlockBatches in DB to only include 2 most recent for this BtUser.
Deleting BlockBatch items will clean up Blocks table automatically via cascade.

@BooDoo BooDoo changed the title Trim Old Blocks (#59) Trim Old Blocks Oct 12, 2014
@@ -74,6 +76,33 @@ function updateBlocks(user) {
logger.error(err);
}).success(function(blockBatch) {
fetchAndStoreBlocks(blockBatch, user.access_token, user.access_token_secret);
destroyOldBlocks(user);
Copy link
Owner

Choose a reason for hiding this comment

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

destroyOldBlocks should only get called when we are finished with a batch and set the 'complete' flag. Otherwise we can wind up with the most recently collected batches being incomplete ones.

@jsha
Copy link
Owner

jsha commented Oct 13, 2014

BTW, this has to wait on submission until #102 is in, or the delete cascades won't actually work.

Note: If you're running locally you may already have these foreign key constraints from sequelize.sync(). The prod instance doesn't because the associations between models were added after initial DB creation.

@BooDoo
Copy link
Contributor Author

BooDoo commented Oct 13, 2014

Moved destroyOldBlocks call to success callback for new BlockBatch being finalized/saved; will wait on review and acceptance of 102 for rebase.

Call destroyOldBlocks in finalizeBlockBatch to remove all but two latest
BlockBatches for user when a new BlockBatch is retrieved successfully.
@BooDoo
Copy link
Contributor Author

BooDoo commented Oct 17, 2014

Rebased on master (foreign key constraints, store size on BlockBatch, &c)

@@ -168,11 +169,45 @@ function finalizeBlockBatch(blockBatch) {
blockBatch.save().error(function(err) {
logger.error(err);
}).success(function(blockBatch) {
// Prune older BlockBatches for this user from the DB.
BtUser.find({
Copy link
Owner

Choose a reason for hiding this comment

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

I would simplify this by leaving out the BtUser.find. Just pass the source_uid to destroyOldBlocks, and it can do a

BlockBatch.findAll({
  where: { source_uid: source_uid }
}

@BooDoo
Copy link
Contributor Author

BooDoo commented Oct 19, 2014

For some reason I thought we might want access to the BtUser record in destroyOldBlocks but there's no good case for that. Changed.

@jsha
Copy link
Owner

jsha commented Oct 19, 2014

One more small thing: Could you check if the offset and order options work for .destroy()? I.e. if you replaced the outermost BlockBatch.findAll with a BlockBatch.destroy(...offset: 2, order: 'id DESC'), would that work?

Also, can you double check the SQL output? By my understanding, the destroy command you have is missing a where clause. But I might misunderstand the destroy() API.

@BooDoo
Copy link
Contributor Author

BooDoo commented Oct 19, 2014

Destroy's first argument is a WHERE object, see here

You can't provide offset in LIMIT clause of mySQL DELETE query; best you can do is hack around it using subquery a la:

DELETE FROM `BlockBatches` 
WHERE `id` IN (
  SELECT `id` FROM (
    SELECT `id`
    FROM `BlockBatches`
    WHERE `source_uid` = 99999999
    ORDER BY `id` DESC
    LIMIT 2, 50000
  ) AS ids);

which isn't performant or legible!

@jsha
Copy link
Owner

jsha commented Oct 19, 2014

Excellent, thanks for looking into it! Merging.

jsha added a commit that referenced this pull request Oct 19, 2014
@jsha jsha merged commit 8ed19c1 into jsha:master Oct 19, 2014
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

Successfully merging this pull request may close these issues.

None yet

2 participants