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

Refactor MailingLists::Subscribers to make it faster #3128

Merged
merged 8 commits into from
Jan 20, 2025

Conversation

amaierhofer
Copy link
Contributor

No description provided.

@amaierhofer amaierhofer force-pushed the feat/sac_1470-refactor-subscriptions-query branch 4 times, most recently from 24fbab5 to a7867be Compare January 10, 2025 09:54
@amaierhofer amaierhofer marked this pull request as ready for review January 10, 2025 09:55
@amaierhofer amaierhofer marked this pull request as draft January 10, 2025 09:56
@amaierhofer
Copy link
Contributor Author

Das script und die zahlen von der PBS sollten wir wahrscheinlich vor dem merge noch entfernen

@amaierhofer amaierhofer force-pushed the feat/sac_1470-refactor-subscriptions-query branch 2 times, most recently from 8a114a5 to e4a68ec Compare January 10, 2025 12:31
@amaierhofer amaierhofer marked this pull request as ready for review January 10, 2025 17:29
@amaierhofer amaierhofer force-pushed the feat/sac_1470-refactor-subscriptions-query branch from e4a68ec to 303b1e3 Compare January 13, 2025 17:01
Copy link
Contributor

@codez codez left a comment

Choose a reason for hiding this comment

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

Etwas Feedback zu den Nebenschauplätzen, die grosse SQL Änderung habe ich nicht überprüft 🤯

app/domain/mailing_lists/subscribers.rb Outdated Show resolved Hide resolved
app/helpers/mailing_lists_helper.rb Outdated Show resolved Hide resolved
app/helpers/mailing_lists_helper.rb Outdated Show resolved Hide resolved
app/helpers/mailing_lists_helper.rb Show resolved Hide resolved
measure_list_performance.rb Outdated Show resolved Hide resolved
@amaierhofer amaierhofer force-pushed the feat/sac_1470-refactor-subscriptions-query branch from 303b1e3 to 7af9b8c Compare January 17, 2025 14:46
@daniel-illi daniel-illi changed the title WIP - refactor MailingLists::Subscribers to make it faster Refactor MailingLists::Subscribers to make it faster Jan 20, 2025
Copy link
Contributor

@daniel-illi daniel-illi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

amaierhofer and others added 8 commits January 20, 2025 11:35
```ruby
require "optparse"

options = {}
OptionParser.new do |opt|
  opt.on("--offset OFFSET") { |o| options[:offset] = o }
  opt.on("--limit LIMIT") { |o| options[:limit] = o }
end.parse!

def now = Process.clock_gettime(Process::CLOCK_MONOTONIC)

require_relative "config/environment"

branch = `git symbolic-ref HEAD | sed -e 's,.*/,,'`.strip
offset = options.fetch(:offset, 0).to_i
limit = options.fetch(:limit, 10).to_i
database = ActiveRecord::Base.connection.instance_variable_get(:@config)[:database]

batch_size = limit / 10
filename = "list-performance-#{database}-#{branch}-#{offset}-#{limit}.csv"

puts "Writing to #{filename}"
File.open(filename, "w") do |file|
  file.write CSV.generate_line(%w[id people seconds])
  MailingList.offset(offset).limit(limit).find_each.with_index do |list, index|
    puts "#{index} of #{limit}" if (index % batch_size) == 0
    file.flush if (index % batch_size) == 0
    start = now
    size = list.people.size
    file.write CSV.generate_line([list.id, size, (now - start).round(1)])
  end
end
```
@daniel-illi daniel-illi force-pushed the feat/sac_1470-refactor-subscriptions-query branch from dedcb78 to 2cbf413 Compare January 20, 2025 10:35
@daniel-illi daniel-illi enabled auto-merge (squash) January 20, 2025 10:36
@daniel-illi daniel-illi merged commit 70e70cc into master Jan 20, 2025
7 checks passed
@daniel-illi daniel-illi deleted the feat/sac_1470-refactor-subscriptions-query branch January 20, 2025 10:48
@daniel-illi
Copy link
Contributor

Das script und die zahlen von der PBS sollten wir wahrscheinlich vor dem merge noch entfernen

Das script steht nun in der commit message drin statt als file im repo 🙃

@codez
Copy link
Contributor

codez commented Jan 21, 2025

Das script und die zahlen von der PBS sollten wir wahrscheinlich vor dem merge noch entfernen

Das script steht nun in der commit message drin statt als file im repo 🙃

Nicht wirklich, oder? 🙈 Was spricht dagegen, das im Repo abzulegen? Im Commit wird es sicher nie mehr gefunden, da wäre löschen wohl noch die bessere Option. 🥇 für die verkehrte Verwendung von Git.

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.

ABOS: Mailinglisten Personenabfrage beschleunigen
3 participants