launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27219
[Merge] ~cjwatson/launchpad:git-auto-repack-frequency into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:git-auto-repack-frequency into launchpad:master.
Commit message:
Avoid auto-repacking the same repository too often
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1933079 in Launchpad itself: "answers.launchpad.net timestamp of changes of comment shown in wrong time zone"
https://bugs.launchpad.net/launchpad/+bug/1933079
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/404530
We don't want to issue automatic repacks for the same Git repository too often: if a repository keeps needing to be repacked on subsequent runs of the cron job, it's much more likely to be because the repack queue hasn't caught up yet than because it's accumulated much more data. Throttle automatic repacks of any single repository to once a week.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-auto-repack-frequency into launchpad:master.
diff --git a/lib/lp/answers/templates/questionmessage-display.pt b/lib/lp/answers/templates/questionmessage-display.pt
index c1ed29a..49973ab 100644
--- a/lib/lp/answers/templates/questionmessage-display.pt
+++ b/lib/lp/answers/templates/questionmessage-display.pt
@@ -24,7 +24,7 @@
<div class='message-revision-item'>
<div class='message-revision-title'>
<a class="js-action">
- Revision #{revision}, created at {date_created}
+ Revision #{revision}, created at {date_created_display}
</a>
</div>
<div class='message-revision-body'>{content}</div>
diff --git a/lib/lp/bugs/templates/bugcomment-box.pt b/lib/lp/bugs/templates/bugcomment-box.pt
index 34e5789..1e1882a 100644
--- a/lib/lp/bugs/templates/bugcomment-box.pt
+++ b/lib/lp/bugs/templates/bugcomment-box.pt
@@ -24,7 +24,7 @@
<div class='message-revision-item'>
<div class='message-revision-title'>
<a class="js-action">
- Revision #{revision}, created at {date_created}
+ Revision #{revision}, created at {date_created_display}
</a>
</div>
<div class='message-revision-body'>{content}</div>
diff --git a/lib/lp/code/scripts/repackgitrepository.py b/lib/lp/code/scripts/repackgitrepository.py
index 1ae66f5..1e59b25 100644
--- a/lib/lp/code/scripts/repackgitrepository.py
+++ b/lib/lp/code/scripts/repackgitrepository.py
@@ -5,15 +5,21 @@
__metaclass__ = type
+from datetime import timedelta
+
from psycopg2.extensions import TransactionRollbackError
import six
-from storm.expr import Or
+from storm.expr import (
+ Cast,
+ Or,
+ )
import transaction
from lp.code.enums import GitRepositoryStatus
from lp.code.errors import CannotRepackRepository
from lp.code.model.gitrepository import GitRepository
from lp.services.config import config
+from lp.services.database.constants import UTC_NOW
from lp.services.database.interfaces import IStore
from lp.services.looptuner import (
LoopTuner,
@@ -37,6 +43,10 @@ class RepackTunableLoop(TunableLoop):
self.store = IStore(GitRepository)
def findRepackCandidates(self):
+ threshold_date = (
+ UTC_NOW - Cast(
+ timedelta(minutes=config.codehosting.auto_repack_frequency),
+ "interval"))
repos = self.store.find(
GitRepository,
Or(
@@ -45,6 +55,9 @@ class RepackTunableLoop(TunableLoop):
GitRepository.pack_count >=
config.codehosting.packs_threshold,
),
+ Or(
+ GitRepository.date_last_repacked == None,
+ GitRepository.date_last_repacked < threshold_date),
GitRepository.status == GitRepositoryStatus.AVAILABLE,
GitRepository.id > self.start_at,
).order_by(GitRepository.id)
@@ -68,7 +81,7 @@ class RepackTunableLoop(TunableLoop):
for repo in repackable_repos:
try:
if self.dry_run:
- print ('Would repack %s' % repo.identity)
+ print('Would repack %s' % repo.identity)
else:
self.logger.info(
'Requesting automatic git repository repack for %s.'
diff --git a/lib/lp/code/scripts/tests/test_repack_git_repositories.py b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
index 53a77c5..9314997 100644
--- a/lib/lp/code/scripts/tests/test_repack_git_repositories.py
+++ b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
@@ -2,6 +2,11 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test the repack_git_repositories script."""
+
+from datetime import (
+ datetime,
+ timedelta,
+ )
import logging
import threading
from wsgiref.simple_server import (
@@ -9,6 +14,7 @@ from wsgiref.simple_server import (
WSGIRequestHandler,
)
+import pytz
import transaction
from zope.security.proxy import removeSecurityProxy
@@ -85,12 +91,12 @@ class TestRequestGitRepack(TestCaseWithFactory):
'in this run of the Automated Repack Job', err)
transaction.commit()
- def runScript_with_Turnip(self):
+ def runScript_with_Turnip(self, expected_count=1):
transaction.commit()
(ret, out, err) = run_script('cronscripts/repack_git_repositories.py')
self.assertIn(
- 'Requested 1 automatic git repository repacks '
- 'out of the 1 qualifying for repack.', err)
+ 'Requested a total of %d automatic git repository repacks '
+ 'in this run of the Automated Repack Job.' % expected_count, err)
transaction.commit()
def makeTurnipServer(self):
@@ -149,7 +155,7 @@ class TestRequestGitRepack(TestCaseWithFactory):
self.makeTurnipServer()
- self.runScript_with_Turnip()
+ self.runScript_with_Turnip(expected_count=10)
for i in range(10):
self.assertIsNotNone(repo[i].date_last_repacked)
@@ -228,6 +234,32 @@ class TestRequestGitRepack(TestCaseWithFactory):
self.assertEqual(repacker.num_repacked, 6)
self.assertEqual(repacker.start_at, 6)
+ def test_auto_repack_frequency(self):
+ self.makeTurnipServer()
+
+ repo = self.factory.makeGitRepository()
+ removeSecurityProxy(repo).loose_object_count = (
+ config.codehosting.loose_objects_threshold + 50)
+ removeSecurityProxy(repo).pack_count = (
+ config.codehosting.packs_threshold + 5)
+ self.assertIsNone(repo.date_last_repacked)
+
+ # An initial run requests a repack.
+ self.runScript_with_Turnip(expected_count=1)
+ self.assertIsNotNone(repo.date_last_repacked)
+
+ # A second run does not request a repack, since the repository
+ # already had a repack requested recently.
+ self.runScript_with_Turnip(expected_count=0)
+ self.assertIsNotNone(repo.date_last_repacked)
+
+ # If we pretend that the last repack request was long enough ago,
+ # then a third run requests another repack.
+ removeSecurityProxy(repo).date_last_repacked = (
+ datetime.now(pytz.UTC) -
+ timedelta(minutes=config.codehosting.auto_repack_frequency + 1))
+ self.runScript_with_Turnip(expected_count=1)
+
def test_auto_repack_findRepackCandidates(self):
repacker = RepackTunableLoop(self.log, None)
@@ -240,7 +272,6 @@ class TestRequestGitRepack(TestCaseWithFactory):
for i in range(3):
repo.append(self.factory.makeGitRepository())
- transaction.commit()
# we should only have 7 candidates at this point
self.assertEqual(7, len(list(repacker.findRepackCandidates())))
diff --git a/lib/lp/code/templates/codereviewcomment-header.pt b/lib/lp/code/templates/codereviewcomment-header.pt
index 3d2b976..3511b9f 100644
--- a/lib/lp/code/templates/codereviewcomment-header.pt
+++ b/lib/lp/code/templates/codereviewcomment-header.pt
@@ -13,7 +13,7 @@
<div class='message-revision-item'>
<div class='message-revision-title'>
<a class="js-action">
- Revision #{revision}, created at {date_created}
+ Revision #{revision}, created at {date_created_display}
</a>
</div>
<div class='message-revision-body'>{content}</div>
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 7ead01c..7795496 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -381,6 +381,10 @@ git_macaroon_secret_key: none
loose_objects_threshold: 4350
packs_threshold: 30
+# Minimum time in minutes between doing automatic repacks of any given Git
+# repository.
+auto_repack_frequency: 10080
+
[codeimport]
# Where the Bazaar imports are stored.
diff --git a/lib/lp/services/messages/interfaces/messagerevision.py b/lib/lp/services/messages/interfaces/messagerevision.py
index aa0ff0a..43f0c9a 100644
--- a/lib/lp/services/messages/interfaces/messagerevision.py
+++ b/lib/lp/services/messages/interfaces/messagerevision.py
@@ -25,6 +25,7 @@ from zope.schema import (
Datetime,
Int,
Text,
+ TextLine,
)
from lp import _
@@ -55,6 +56,12 @@ class IMessageRevisionView(Interface):
title=_("The time when this message revision was created."),
required=True, readonly=True))
+ date_created_display = exported(TextLine(
+ title=_(
+ "The time when this message revision was created, rendered in the "
+ "user's timezone for the web UI."),
+ required=True, readonly=True))
+
date_deleted = exported(Datetime(
title=_("The time when this message revision was created."),
required=False, readonly=True))
diff --git a/lib/lp/services/messages/javascript/messages.edit.js b/lib/lp/services/messages/javascript/messages.edit.js
index c6e79b6..56762e8 100644
--- a/lib/lp/services/messages/javascript/messages.edit.js
+++ b/lib/lp/services/messages/javascript/messages.edit.js
@@ -201,8 +201,6 @@ YUI.add('lp.services.messages.edit', function(Y) {
var content = "";
revisions.forEach(function(rev) {
var attrs = rev.getAttrs();
- var date_created = new Date(attrs.date_created);
- attrs.date_created = date_created.toLocaleString();
attrs.content = module.htmlify_msg(attrs.content);
content += Y.Lang.sub(template, attrs);
});
diff --git a/lib/lp/services/messages/javascript/tests/test_messages.edit.html b/lib/lp/services/messages/javascript/tests/test_messages.edit.html
index e6a27c2..9d394c2 100644
--- a/lib/lp/services/messages/javascript/tests/test_messages.edit.html
+++ b/lib/lp/services/messages/javascript/tests/test_messages.edit.html
@@ -74,7 +74,7 @@ GNU Affero General Public License version 3 (see the file LICENSE).
<!-- The description of the revision, that will expand the
revision content once clicked. -->
<div class='message-revision-title'>
- <a class="js-action">Revision #{revision}, created at {date_created}</a>
+ <a class="js-action">Revision #{revision}, created at {date_created_display}</a>
</div>
<!-- The revision content itself -->
<div class='message-revision-body'>{content}</div>
@@ -119,7 +119,7 @@ GNU Affero General Public License version 3 (see the file LICENSE).
<script type="text/template">
<div class='message-revision-item'>
<div class='message-revision-title'>
- <a class="js-action">Revision #{revision}, created at {date_created}</a>
+ <a class="js-action">Revision #{revision}, created at {date_created_display}</a>
</div>
<div class='message-revision-body'>{content}</div>
</div>
diff --git a/lib/lp/services/messages/javascript/tests/test_messages.edit.js b/lib/lp/services/messages/javascript/tests/test_messages.edit.js
index f5cbd39..d9644f0 100644
--- a/lib/lp/services/messages/javascript/tests/test_messages.edit.js
+++ b/lib/lp/services/messages/javascript/tests/test_messages.edit.js
@@ -190,11 +190,11 @@ YUI.add('lp.services.messages.edit.test', function(Y) {
"entries": [{
"revision": 1,
"content": "content 1",
- "date_created": "2021-05-10T19:41:36.102749+00:00"
+ "date_created_display": "2021-05-10 19:41:36 UTC"
}, {
"revision": 2,
"content": "content 2",
- "date_created": "2021-05-11T22:17:11.000123+00:00"
+ "date_created_display": "2021-05-11 22:17:11 UTC"
}]
};
module.lp_client.io_provider.success({
@@ -216,13 +216,11 @@ YUI.add('lp.services.messages.edit.test', function(Y) {
revisions.each(function(rev, i) {
// Entries are shown in reverse order.
var entry = response.entries[response.entries.length - i -1];
- var date_created = new Date(entry.date_created);
- entry['formatted_date'] = date_created.toLocaleString();
var title = rev.one('.message-revision-title');
var body = rev.one('.message-revision-body');
var expected_title = Y.Lang.sub(
'<a class="js-action">' +
- 'Revision #{revision}, created at {formatted_date}' +
+ 'Revision #{revision}, created at {date_created_display}' +
'</a>',
entry);
Y.Assert.areSame(
diff --git a/lib/lp/services/messages/model/messagerevision.py b/lib/lp/services/messages/model/messagerevision.py
index 535ff66..5dc20e3 100644
--- a/lib/lp/services/messages/model/messagerevision.py
+++ b/lib/lp/services/messages/model/messagerevision.py
@@ -20,6 +20,7 @@ from storm.locals import (
)
from zope.interface import implementer
+from lp.app.browser.tales import DateTimeFormatterAPI
from lp.services.database.constants import UTC_NOW
from lp.services.database.interfaces import IStore
from lp.services.database.stormbase import StormBase
@@ -87,6 +88,10 @@ class MessageRevision(StormBase):
def content(self):
return '\n\n'.join(i.content for i in self.chunks)
+ @property
+ def date_created_display(self):
+ return DateTimeFormatterAPI(self.date_created).datetime()
+
def deleteContent(self):
store = IStore(self)
store.find(MessageRevisionChunk, message_revision=self).remove()
diff --git a/lib/lp/services/messages/tests/test_messagerevision.py b/lib/lp/services/messages/tests/test_messagerevision.py
index 80d1a14..19cc931 100644
--- a/lib/lp/services/messages/tests/test_messagerevision.py
+++ b/lib/lp/services/messages/tests/test_messagerevision.py
@@ -14,6 +14,7 @@ from testtools.matchers import (
from zope.security.interfaces import Unauthorized
from zope.security.proxy import ProxyFactory
+from lp.app.browser.tales import DateTimeFormatterAPI
from lp.bugs.interfaces.bugmessage import IBugMessage
from lp.services.database.interfaces import IStore
from lp.services.database.sqlbase import get_transaction_timestamp
@@ -85,6 +86,7 @@ class TestMessageRevisionAPI(MessageTypeScenariosMixin, TestCaseWithFactory):
msg = self.makeMessage(content="initial content")
msg.editContent("new content 1")
msg.editContent("final content")
+ dates_created = [revision.date_created for revision in msg.revisions]
ws = self.getWebservice(self.person)
url = self.getMessageAPIURL(msg)
ws_message = ws.get(url).jsonBody()
@@ -95,37 +97,45 @@ class TestMessageRevisionAPI(MessageTypeScenariosMixin, TestCaseWithFactory):
"total_size": Equals(2)}))
self.assertThat(revisions["entries"], MatchesListwise([
ContainsDict({
- "date_created": Not(Is(None)),
- "date_deleted": Is(None),
"content": Equals("initial content"),
+ "date_created": Equals(dates_created[0].isoformat()),
+ "date_created_display": Equals(
+ DateTimeFormatterAPI(dates_created[0]).datetime()),
+ "date_deleted": Is(None),
"self_link": EndsWith("/revisions/1")
- }),
+ }),
ContainsDict({
- "date_created": Not(Is(None)),
- "date_deleted": Is(None),
"content": Equals("new content 1"),
+ "date_created": Equals(dates_created[1].isoformat()),
+ "date_created_display": Equals(
+ DateTimeFormatterAPI(dates_created[1]).datetime()),
+ "date_deleted": Is(None),
"self_link": EndsWith("/revisions/2")
- })]))
+ })]))
def test_get_single_revision(self):
msg = self.makeMessage(content="initial content")
msg.editContent("new content 1")
+ date_created = msg.revisions[0].date_created
ws = self.getWebservice(self.person)
with person_logged_in(self.person):
revision_url = api_url(msg.revisions[0])
revision = ws.get(revision_url).jsonBody()
self.assertThat(revision, ContainsDict({
- "date_created": Not(Is(None)),
- "date_deleted": Is(None),
- "content": Equals("initial content"),
- "self_link": EndsWith("/revisions/1")
+ "content": Equals("initial content"),
+ "date_created": Equals(date_created.isoformat()),
+ "date_created_display": Equals(
+ DateTimeFormatterAPI(date_created).datetime()),
+ "date_deleted": Is(None),
+ "self_link": EndsWith("/revisions/1")
}))
def test_delete_revision_content(self):
msg = self.makeMessage(content="initial content")
msg.editContent("new content 1")
msg.editContent("final content")
+ dates_created = [revision.date_created for revision in msg.revisions]
with person_logged_in(self.person):
revision_url = api_url(msg.revisions[0])
@@ -136,16 +146,19 @@ class TestMessageRevisionAPI(MessageTypeScenariosMixin, TestCaseWithFactory):
revision = ws.get(revision_url).jsonBody()
self.assertThat(revision, ContainsDict({
- "date_created": Not(Is(None)),
- "date_deleted": Not(Is(None)),
"content": Equals(""),
+ "date_created": Equals(dates_created[0].isoformat()),
+ "date_created_display": Equals(
+ DateTimeFormatterAPI(dates_created[0]).datetime()),
+ "date_deleted": Not(Is(None)),
"self_link": EndsWith("/revisions/1")
- }))
+ }))
def test_delete_revision_content_denied_for_non_owners(self):
msg = self.makeMessage(content="initial content")
msg.editContent("new content 1")
msg.editContent("final content")
+ dates_created = [revision.date_created for revision in msg.revisions]
someone_else = self.factory.makePerson()
with person_logged_in(self.person):
@@ -157,9 +170,10 @@ class TestMessageRevisionAPI(MessageTypeScenariosMixin, TestCaseWithFactory):
revision = ws.get(revision_url).jsonBody()
self.assertThat(revision, ContainsDict({
- "date_created": Not(Is(None)),
- "date_deleted": Is(None),
"content": Equals("initial content"),
+ "date_created": Equals(dates_created[0].isoformat()),
+ "date_created_display": Equals(
+ DateTimeFormatterAPI(dates_created[0]).datetime()),
+ "date_deleted": Is(None),
"self_link": EndsWith("/revisions/1")
- }))
-
+ }))
