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

Performanceupdate für URL Addon URLs #277

Merged
merged 13 commits into from
Jul 29, 2020
Merged

Performanceupdate für URL Addon URLs #277

merged 13 commits into from
Jul 29, 2020

Conversation

TobiasKrais
Copy link
Member

Dieses Update stellt 3 neue Methoden in der search_it Klasse zur Verfügung. Alle drei vergleichen den SearchI It Index mit der URL Tabelle und beschränken den Update des Suchindex auf die gefundenen Änderungen:

  • indexNewURLs(): fügt neue URLs aus dem URL Addon zum Suchindex hinzu
  • indexUpdatedURLs(): aktualisiert aktualisierte URLs im Suchindex
  • unindexDeletedURLs(): löscht gelöschte URLs aus dem Suchindex

Die Cronjob Klasse im PR nutzt diese Methoden und ist dadurch erheblich performanter. Außerdem bereitet dieser PR einen kommenden EP im URL Addon vor, der über Änderungen in der URL Tabelle informiert. Der EP teilt aber nicht mit, welche Änderungen dies sind.

@TobiasKrais TobiasKrais changed the title Performanceupdate für URL Addon Performanceupdate für URL Addon URLs Apr 25, 2020
@TobiasKrais
Copy link
Member Author

Update basiert auf diesem PR: #272

Copy link
Member

@alxndr-w alxndr-w left a comment

Choose a reason for hiding this comment

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

Ich kann das nicht beurteilen.

@tyrant88
Copy link
Member

3 Funktionen und 1 Änderung an der Index-Tabelle? Das finde ich nicht gut.
Ist für mich nur ne Krücke, weil es keinen EP im URL AddOn gibt.

@TobiasKrais
Copy link
Member Author

Soll ich eine einzige Funktion daraus machen?

Der passende EP im URL Addon kommt.

Hintergrund ist bei mir, dass ich in einem Projekt wenn die Übersetzungen fertig sind 1500 URLs habe und beim Conjob über der max_execution_time liege. Dann kann ich den Index nur noch manuell aufbauen. Mit diesem Update ist das Limit locker unterboten.

@tyrant88
Copy link
Member

Ja, das war mir schon klar, dass es ein konkretes Problem bei dir löst.
Aber ich bevorzuge eine allgemeingültigere Lösung. Das ist der EP.

@alxndr-w
Copy link
Member

Ich hol schon Mal Popcorn

@TobiasKrais
Copy link
Member Author

Definitiv. Der EP ist die Lösung. Die Diskussion läuft auch schon: tbaddade/redaxo_url#185 (comment). Der PR ist die Vorarbeit für den angedachten EP.

if(rex_addon::get('search_it')->getConfig('index_url_addon') && search_it_isUrlAddOnAvailable()) {
$sql = rex_sql::factory();
$sql->setQuery("SELECT url.url_hash, url.article_id, url.clang_id, url.profile_id, url.data_id FROM `". search_it_getUrlAddOnTableName() ."` as url "
. "LEFT JOIN `". $this->tempTablePrefix ."search_it_index` AS search_it ON url.url_hash = search_it.fid "
Copy link
Member

Choose a reason for hiding this comment

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

muss jetzt überall 'self::getTempTablePrefix()' und 'self::getTablePrefix()' heissen. Das hatte Tim Filler geliefert letzte Woche...

@tyrant88
Copy link
Member

Äh, sorry ich meinte nur in deinen geänderten Zeilen, das ändern... dann hätte man es mit der aktuellen einfach mergen können... Falls ich git richtig verstehe...

@@ -106,8 +106,8 @@ function __construct($_clang = false, $_loadSettings = true, $_useStopwords = tr
}

$this->setClang($_clang);
$this->tablePrefix = rex::getTablePrefix();
$this->tempTablePrefix = rex::getTablePrefix().rex::getTempPrefix();
self::getTablePrefix() = rex::getTablePrefix();
Copy link
Member

Choose a reason for hiding this comment

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

Das muss raus. Es ist ja keine statische memberVaribale mehr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ist gemacht. Konflikt ist behoben.

@TobiasKrais
Copy link
Member Author

Git merkt das, wenn in beiden Zweigen die gleichen Änderungen gemacht wurden. Ich hab mit den gesamten Commit angeschaut. Das müsste so funktionieren. Konflikte kann man ja sowieso mit "resolve conflicts" lösen. Wenn du möchtest, beschränke ich die Änderungen auf den neuen Code.

@tyrant88
Copy link
Member

kannst du das dann noch mal testen, wenn ich gemergt habe?

@TobiasKrais
Copy link
Member Author

Ja klar.

@tyrant88 tyrant88 merged commit 7214467 into master Jul 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the lastindex branch July 29, 2020 15:25
@tyrant88 tyrant88 mentioned this pull request Oct 13, 2020
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.

3 participants