Skip to content
This repository has been archived by the owner on Oct 29, 2019. It is now read-only.

Fix alter_time format when deleting old data (bsc#1112967) #688

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

mjura
Copy link
Contributor

@mjura mjura commented Oct 25, 2018

No description provided.

")
[:events, :returns].each do |table|
ActiveRecord::Base.connection.execute("
delete from `salt_#{table}` where alter_time < #{date.to_i};
delete from `salt_#{table}` where unix_timestamp(alter_time) < #{date.to_i};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rethink this a bit more, because with this we could effectively delete events that weren't processed at all (processed_at IS NULL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've added it

@jordimassaguerpla
Copy link
Member

I see I am a reviewer. I will wait for ereslibre approval and then I will give you my approval .

@mjura mjura force-pushed the alter_time branch 2 times, most recently from 06944b1 to bfbd99c Compare October 29, 2018 12:58
@mjura
Copy link
Contributor Author

mjura commented Oct 29, 2018

I've updated also brakeman ignore config file

delete from `salt_#{table}` where alter_time < #{date.to_i};
ActiveRecord::Base.connection.execute("
delete from `salt_events` where unix_timestamp(alter_time) < #{date.to_i}
and processed_at is NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be processed_at is not null; so we are sure that we are cleaning events that have been processed (their processed_at column is not NULL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it

Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mjura

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants