-
Notifications
You must be signed in to change notification settings - Fork 8
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
Rework gbif cleanup #1220
Rework gbif cleanup #1220
Conversation
event.destroy! if event.present? | ||
# delete event records from mysql | ||
sql = ActiveRecord::Base.sanitize_sql_array(["DELETE FROM events WHERE id IN (?)", ids]) | ||
ActiveRecord::Base.connection.execute(sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we delete the events using raw query, we are skipping the after_commit on: [:destroy]
callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashwinisukale yes this is by design as I will be using the bulk api to delete the documents in the events index.
|
||
response = Event.query(query, filter.merge(page: { size: 1, cursor: [] })) | ||
|
||
if response.size.positive? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should verify if response.present? && response.respond_to?(:size) && response.size.positive?
response present, incase of an error
while response.size.positive? && delete_count < max_delete_count | ||
response = Event.query(query, filter.merge(page: { size: size, cursor: cursor })) | ||
|
||
break unless response.size.positive? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can make little readable by checking it like this break if response.size.zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, there are some refactoring changes needed, but as you said we will be deleting this code so no need to worry on refactoring part.
Thanks,
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Purpose
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines: