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

Js translations #147

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Js translations #147

wants to merge 16 commits into from

Conversation

3eggert
Copy link

@3eggert 3eggert commented Mar 2, 2020

I added a new export format which writes the translations in a js file to use it with java script.

In addition I wrote a mechanism to track which keys are actually used. The usage count and date of the last usage is written to the database and displayed in the localization view. I meant to help to delete unused keys. To enable this feature I added this config option:
# Persist global_hits_counter every 1000 lookups, set to nil or comment out to disable Lit.persit_global_hits_count = 1000

I also added this config option to speed up the startup, for cases where lit is not needed:
# Initialize lit unless it ist disabled by setting the SKIP_LIT enviroment variable. Disabling is usefull to speed up the startup, for example to execute migrations Lit.init unless ENV["SKIP_LIT"] == "1"

@mlitwiniuk
Copy link
Member

In the end I forgot to answer your issue, sorry for that.

Regarding this PR - I like the idea, but... persit_global_hits_counters must be scheduled for some kind of background processor (probably even ActiveJob will do the job, as we'e dropped Rails <5). We're maintaining projects that have thousands of localizations, saving data to db even every 1000th initialization will still add a lot of unnecessary overhead.

@3eggert
Copy link
Author

3eggert commented Mar 2, 2020

Thanks for your reply. Our system has about 4800 keys and I did not notice any lag or other issues til now. I have indeed big performance issue in startup. I have at the moment no idea why it is so slow.

Never the less performance improvements are a good idea. I'll have a look.

---- edit:
I'm afraid I misunderstood you, why there is overhead every 1000th initialization?
I persist the counts every 1000 (configurable) I18n requests. The initialization was also brutally slow before my changes, this is the reason I added the possibility to disable lit for the parts of the deployment which do not require it.

@3eggert
Copy link
Author

3eggert commented Mar 2, 2020

Despite being unsure if I understand the issue you mentioned right, I moved persit_global_hits_counters into a ActiveJob.
Please have a look.

@mlitwiniuk mlitwiniuk self-requested a review May 31, 2020 19:32
@mlitwiniuk mlitwiniuk self-assigned this May 31, 2020
Copy link
Member

@mlitwiniuk mlitwiniuk left a comment

Choose a reason for hiding this comment

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

@3eggert Hi, sorry for late input, but... well, I don't have proper excuse. Will you fix those "issues" and add some tests or should I fetch your changes and do it on my own? I'm ok with either

@@ -7,7 +7,7 @@ translating app by non-technicals.

Highly inspired by Copycopter by thoughtbot.

[![travis status](https://travis-ci.org/prograils/lit.svg)](https://travis-ci.org/prograils/lit)
[![travis status](https://api.travis-ci.org/3eggert/lit.svg?branch=js_translations)](https://api.travis-ci.org/3eggert/lit.svg?branch=js_translations)
Copy link
Member

Choose a reason for hiding this comment

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

please revert that

Suggested change
[![travis status](https://api.travis-ci.org/3eggert/lit.svg?branch=js_translations)](https://api.travis-ci.org/3eggert/lit.svg?branch=js_translations)
[![travis status](https://travis-ci.org/prograils/lit.svg)](https://travis-ci.org/prograils/lit)


def execute
@update_array.each do |a|
Lit::LocalizationKey.find(a[0]).update_columns(usage_count: a[1], used_last_at: Time.now)
Copy link
Member

Choose a reason for hiding this comment

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

as per my comment to Lit::Cache

Suggested change
Lit::LocalizationKey.find(a[0]).update_columns(usage_count: a[1], used_last_at: Time.now)
Lit::LocalizationKey.where(id: a[0]).update_all('usage_count = usage_count + ?, user_last_at = ?', a[1], Time.now)

@@ -143,6 +144,18 @@ def get_global_hits_counter(key)
@hits_counter['global_hits_counter.' + key]
end

def persit_global_hits_counters
Copy link
Member

Choose a reason for hiding this comment

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

Over time this will grow - I suggest clearing that with every call of #persit_global_hits_counters and starting from 0 again. Then PersitGlobalHitsCountersService should make db sum count with added value, rather than set it. It will also make hits counter independent of redis (ie. redis can be flushed every now and then).

def persit_global_hits_counters
update_array = []
@hits_counter.each do |k,v|
if k.match?(/^global_hits_counter/)
Copy link
Member

Choose a reason for hiding this comment

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

It might be works considering adding new method to Redis and Hash adapter, that will return all keys matching given criteria, ie. redis has this already built in: https://redis.io/commands/keys

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.

2 participants