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

autoupdate urls from url addon 2.x #289

Merged
merged 1 commit into from
Jul 29, 2020
Merged

autoupdate urls from url addon 2.x #289

merged 1 commit into from
Jul 29, 2020

Conversation

TobiasKrais
Copy link
Member

Mit diesem Update können URLs des URL 2.x Addons automatisch indexiert werden.

@TobiasKrais
Copy link
Member Author

Relevant sind Zeilen 61 bis 77. Bei den anderen habe ich unnötige Leerstellen / Tabs entfernt.

@TobiasKrais TobiasKrais requested a review from tyrant88 July 28, 2020 13:32
@TobiasKrais
Copy link
Member Author

Braucht noch diesen PR: #277

Copy link
Member

@tyrant88 tyrant88 left a comment

Choose a reason for hiding this comment

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

In deinem alten commit sind doch noch die Änderungen mit dem "lastindexed"-Datum drin, oder?
Sollte die Sache mit dem EP das nicht ersetzen?

@TobiasKrais
Copy link
Member Author

Du meinst im PR #277? Ja, das ist korrekt und wird benötigt. Dadurch kann man feststellen, ob das Datum der letzten Änderung des Datensatzes im URL Addon neuer ist als das Datum der letzten Indexierung. Nur dann erfolgt eine erneute Indexierung.

Lässt man den Teil weg, würde jedes mal die komplette URL Tabelle reindexiert werden. Ich habe hier 800 URLs am Start. In so einem Fall wäre das Backend erst mal ein paar Minuten nicht mehr erreichbar wenn man den Code weg lässt.

Vielleicht noch als Hintergrundinfo: das URL Addon ruft den EP nicht nur für die geänderte, neue oder gelöschte URL auf. Das URL Addon befüllt jedes mal die komplette URL Tabelle neu. Bei meinen 800 URLs wird der EP 801 mal aufgerufen. 1 mal zum Löschen der URL Tabelle und dann 800 mal für jede einzelne URL.

@tbaddade
Copy link
Member

Das URL Addon befüllt jedes mal die komplette URL Tabelle neu.

Das stimmt so nicht. Die Urls können pro Datensatz (ein Datensatz kann mehrere Urls besitzen), pro Gruppe (wenn sich der Artikelname in der Struktur ändert) oder komplett neu erstellt werden (Cache löschen zum Bsp.).

@TobiasKrais
Copy link
Member Author

Danke für den Hinweis, Thomas! Da hast du natürlich Recht. Der Punkt für uns ist, dass uns der EP nicht auf eine einzelne Änderung hinweisen kann die es zu reindexieren gilt. Deshalb setzen wir im Commit den Trigger und arbeiten ihn ganz zum Schluss mit dem letzten EP ab.

@tyrant88
Copy link
Member

D.h. du brauchst das Datum in der index-tabelle, damit nur wirklich geänderte reindexiert werden,
den EP damit die Reindexierung angestoßen wird und den config-Value, damit es nur einmal nach dem Anstoßen gemacht wird?

@TobiasKrais
Copy link
Member Author

Ja, ganz genau so ist es. Der Config Value existiert auch nur für die Zeit in der das Skript läuft.

Denkbar wäre auch noch ein Limit einzuführen, so dass z.B. maximal 50 Seiten auf einmal indexiert werden.

@tyrant88
Copy link
Member

Meine Idee war ja noch, das Reindexieren von dem Request zu lösen, in dem eben nur der config-Value auf einen timstamp in der Zukunft gesetzt und später ausgelesen wird.
Jetzt würde ja jede Änderung an der URL-Tabelle eine Änderung auslösen.

Wie ist das denn, wenn man übliche Änderungen mit dem URL-AddOn vornimmt? Wie oft würde die Reindexierung dann triggern? Oder ist sie dann immer so "klein", dass es nicht stört?

@TobiasKrais
Copy link
Member Author

TobiasKrais commented Jul 29, 2020

Üblicherweise merkt man gar nicht, dass im Hintergrund etwas passiert. Es ist genau so wie wenn ein Artikel geändert wird.

Nimmt man Änderungen am URL Addon vor, kann es in wenigen Fällen die Performance drücken. Warum? Angenommen die generierten URLs eines Profils mit vielen Datensatzen ändern sich alle. Dann werden auch alle URLs neu indexiert, da der Bezug zur alten URL nicht hergestellt werden kann. Genauso auch beim Erstellen eines neuen Profils mit vielen Datensätzen. Alles andere läuft so performant, dass man es nicht merkt.

@tyrant88
Copy link
Member

Warum hast du eigentlich einen config-Value genommen? Wäre nicht eine statische Member-Variable das Richtige?

@TobiasKrais
Copy link
Member Author

Falls die Skriptlaufzeit überschritten wird, wird die Indexierung beim nächsten mal beendet wenn es in der Config gespeichert wird. Als Member Variable kann etwas verloren gehen. Wenn dir aber eine Member Variable lieber ist mach ich das.

@tyrant88
Copy link
Member

Ist ja schon mal ein Argument. Es geht nur in nachgeordneter Linie darum, was mir lieber ist. Erstmal zählen Argumente.

@tyrant88 tyrant88 merged commit b989cc5 into master Jul 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the url-addon-autoupdate branch July 29, 2020 15:26
@tyrant88 tyrant88 mentioned this pull request Oct 13, 2020
@alxndr-w
Copy link
Member

Üblicherweise merkt man gar nicht, dass im Hintergrund etwas passiert.

Fürs Protokoll: Das wäre vielleicht jedoch wünschenswert, wenn man was davon merkt, das auch einordnen zu können.

Beim Ändern von URL-Profilen und beim Ändern von YRewrite-Einstellungen wird eine clientseitige Indexierung angestoßen, indem per GET alle URLs neu angefragt werden. Bei bspw. 300 URLs à 150ms ergibt das 45 Sekunden.

Der Entwickler bekommt darüber jedoch keine visuelle Information - es sieht so aus, als wäre YRewrite oder URL einfach langsam oder hätte sich aufgehängt.

  • Lässt sich der Vorgang noch optimieren, indem z.B. 5 URLs gleichzeitig angestoßen werden?
  • Lässt sich noch eine Meldung zusätzlich ausgeben, solange der PJAX-Spinner sich im Kreis dreht?

@TobiasKrais
Copy link
Member Author

@alxndr-w: Da wir uns in den EP RESPONSE_SHUTDOWN reinhängen, lässt sich da nichts mehr einblenden.

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.

4 participants