Project

General

Profile

Bug #4194

Alvorlig problem med Openseach cache og lokale id'er

Added by Simon Holt 10 months ago. Updated 8 months ago.

Status:
Resolved (tag version)
Priority:
Urgent
Assignee:
Estimated time:
URL med eksempel:
Kategorier:
Integration - Brønd - Data og relationer

Description

Jf. #3905.

Der er stadig problemer med cache af local ids, som medfører at alle request på lokale id'er fejler og vi altid ender med at bruge basis namespace i statuslisterne.

Problemet er her hvor ikke-cachede id'er tilføjes til request: https://github.com/ding2/ding2/blob/master/modules/opensearch/opensearch.client.inc#L422 

Hvis request oprindelig var baseret på lokale id'er, vil request nu blive lavet med identifier i stedet for localIdentifier, da det er setObjectIds() der bruges og ikke setLocalIds(). Det er fordi identifier er den der først tjekkes i TingObjectRequest og bruges hvis den er sat (derfor har denne fejl ingen betydning for request baseret på identifier/PID).

Løsningen må være også at håndtere lokale id'er separat ligesom det gøres ovenfor, når requests bliver konstrueret.

PR følger.

History

#2 Updated by Gitte Barlach 10 months ago

  • Status changed from New to Needs code review
  • Assignee changed from Simon Holt to Jesper Kristensen
  • Priority changed from Normal to Urgent

Hej Jesper 
Kan du se på denne?

#3 Updated by Simon Holt 9 months ago

Har fundet yderligere fejl i caching af request baseret på local id og opdateret PR med rettelser. Har nu testet og verficeret at:

1. På en tom cache laver den et korrekt request på localIdentifier

2. Hvis et identisk getObject request med lokale id'er ligger cachen, rammer den cachen og laver ikke et nyt request.

3. Hvis nogle af objekterne ligger i cachen tager den dem og laver et request på de resterende. De cachede resultat og resultat af request merges korrekt sammen til sidst.

4. I alle tilfælde returneres objekterne korrekt og keyed by PID.

Ifb med mit arbejdet i #3936 opdagede jeg, at et par stykker af de titler jeg testede hvor jeg fik "Error missing title" faktisk ikke var fjernlån, men lokale poster der ikke havde fået det rigtig namespace pga af fejlen her. Hvis en lokal post slås op med basis namespace får man et fejlsvar.

Merge af ovenstående PR løste problemet.

#4 Updated by Jesper Kristensen 9 months ago

@simon arbejder dette sammen med: https://platform.dandigbib.org/issues/4139

som jeg lige ser det er det samme område vi leger rundt i.

#5 Updated by Gitte Barlach 9 months ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jesper Kristensen to Simon Holt

Da PR i #4139 er godkendt og klar, foreslår jeg at vi merger det først. 

#6 Updated by Simon Holt 9 months ago

Er enig i vi merger #4139 først og jeg skal nok sikre Jespers rettelser spiller med mine i rebase.

Men tror vi bliver nødt til at få merget denne her sammen med rettelsen jeg er ved at lave i https://platform.dandigbib.org/issues/3936.

Det er meget vanskeligt at få #3936 til at spille uden rettelsen her, da der sker nogle underlige ting cahcing laget og den cacher slet ikke.

Får også hele tiden følgende fejl som giver en white page:

"Fatal error: Unsupported operand types in /var/www/drupalvm/drupal/web/profiles/ding2/modules/opensearch/opensearch.client.inc on line 108"

PR i denne sag retter også ovenstående fejl.

#7 Updated by Simon Holt 9 months ago

Nå men det viste sig at være et helt andet urelateret problem, der gjorde det vanskeligt og at jeg fik alle de fejl: https://github.com/ding2/ding2/pull/1384/commits/e19de5e7ae722f126a1c85390f4efa583375ea02

Det hele så faktisk ud til at spille ret godt til sidst, så tror ikke det er strengt nødvendigt med rettelsen her.

Meeeeen cachingen af getObject request med lokale id'er fungerer slet ikke, så merges disse rettelser her, burde det give generelt bedre performance på statuslisterne.

Og så er der selvføglelig ""Fatal error: Unsupported operand types"-problemet, som man konstant render i, hvis der bliver lavet et getObject request der ikke finder noget objekter.

#8 Updated by Simon Holt 9 months ago

@Jepser

Jeg har nu testet PR her med dine rettelser fra #4139 merget og det ser ud til at virke fint sammen. Der var selvfølgelig nogle merge-konflikter, men dem kan jeg bare løse når jeg rebaser efter dit PR er blevet merget.

Jeg har også testet begge vores rettelser sammen med min seneste fra #3936 og det spiller bare! For første gang fik jeg faktisk vist informationer om alle de lån/reseveringer jeg burde.

Alle lånestatus siderne virker også til at køre meget bedre og jeg får ikke en masse fejl spyttet ud.

#9 Updated by Simon Holt 9 months ago

@Jepser Lige en enkelt kommentar: https://github.com/ding2/ding2/pull/1364/files#r263461849 

 

 

#10 Updated by Simon Holt 9 months ago

  • Status changed from Reviewed - Needs info/rework to Needs code review
  • Assignee changed from Simon Holt to Gitte Barlach

Vil anbefale at vi får merget disse rettelser ASAP. Den bør patches på som en hotfix eller noget.

For uden de vigtige vigtige rettelser til caching laget indeholder PR også en rettelse til en fatal error som jeg konstant støder på, når jeg sidder og tester. Fejlen udløser en WSOD, så det er bestemt meget alvorligt.

#11 Updated by Gitte Barlach 9 months ago

  • Assignee changed from Gitte Barlach to Jesper Kristensen
  • Target version set to Release 30 - Bugfixes

#12 Updated by Gitte Barlach 9 months ago

  • Target version changed from Release 30 - Bugfixes to Release 30-1 - Place2Book (7.x-4.7.0)

#13 Updated by Jesper Kristensen 9 months ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jesper Kristensen to Simon Holt

Bare et mindre spørgmål. Se github.

#14 Updated by Simon Holt 9 months ago

@Jesper har svaret på Github.

#15 Updated by Jesper Kristensen 9 months ago

  • Status changed from Reviewed - Needs info/rework to Reviewed
  • Assignee changed from Simon Holt to Gitte Barlach

Reviewed og afventer release

#16 Updated by Kasper Garnæs 9 months ago

  • Status changed from Reviewed to Technical test

Merged.

#17 Updated by Gitte Barlach 8 months ago

  • Status changed from Technical test to Resolved (tag version)

Testet på upgrade-fbs med 4.7.0-rc3

Vi (Gitte & Nhi) har ad flere omgange generelt testet søg-vis funktionalitet på sitet, og alt ser ud til at virke ok. Godkender derfor denne. 

Also available in: Atom PDF