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

S3 naar B2 #73

Merged
merged 22 commits into from
Nov 24, 2019
Merged

S3 naar B2 #73

merged 22 commits into from
Nov 24, 2019

Conversation

indiePeeters
Copy link
Contributor

@indiePeeters indiePeeters commented Mar 16, 2019

Hier de pull request om S3 naar B2 te zetten

Alles verliep vrij soepel, alleen dat de library niet de url methode ondersteund. Waardoor het ophalen van de foto url niet kan. Aangezien ik zo snel mogelijk wil beginnen aan de ideal betalingen heb ik voor een snelle oplossing gekozen. Nu wordt url opgebouwd via string concat. Stel B2 kiest er later voor om hun server locatie te veranderen betekend dit dat er een regeltje in de code moet worden aangepast.

@wouterbles
Copy link
Contributor

Is er een reden dat we niet \Storage::disk('b2')->get('filename.txt') gebruiken?

@indiePeeters
Copy link
Contributor Author

Omdat je een RAW file contents terug krijgt. Deze moet je dan encoden met base64. Dit duurde tijdens het uploaden vrij lang en om die reden ook verwijderd bij het ophalen van bestanden.

@wouterbles
Copy link
Contributor

Helder! Heb even gezocht, en volgensmij is de URL gewoon bucket specifiek, zou dus opzich geen probleem moeten zijn. Dan lijkt het me wel zo netjes om de bucket URL even in de .env file te zetten ipv direct in de code. Hoe denk jij hierover?

@indiePeeters
Copy link
Contributor Author

Yes, dat is inderdaad beter, zal het fixen

Signed-off-by: Indie <indiepeeters@hotmail.com>
Signed-off-by: Indie <indiepeeters@hotmail.com>
@indiePeeters
Copy link
Contributor Author

indiePeeters commented Mar 19, 2019

@wouterbles Kan ik deze mergen? Fixed #55 en #61

@wouterbles
Copy link
Contributor

wouterbles commented Mar 20, 2019

Is het nog een idee om een migratie aan te maken die de huidige DB entries in de foto tabel verwijderd? Weet niet of dit best practice is? Maar anders moeten we handmatig gaan kloten op de live server database.

Ook is de seeder nog niet geüpdate denk ik?

Oh en over die bucket url doelde ik eigenlijk op de volledige URL. Het f002 gedeelte wat er voor staat is wat ik uit de docs begreep namelijk ook bucket specifiek.

@marijnvanderhorst
Copy link
Member

Is het nog een idee om een migratie aan te maken die de huidige DB entries in de foto tabel verwijderd? Weet niet of dit best practice is? Maar anders moeten we handmatig gaan kloten op de live server database.

Table truncates zou ik niet in een migration zetten, deze is echt bedoeld voor structurele wijzigingen voor zo ver ik weet.

@indiePeeters
Copy link
Contributor Author

Seeder moet inderdaad opnieuw gemaakt worden. Mij lijkt het handmatig doen voor nu de snelste optie

@wouterbles
Copy link
Contributor

Ben het eens met Marijn, een migratie voor data uit een tabel verwijderen is niet logisch.

@indiePeeters Bedoel je handmatig seeden of handmatig de data uit de tabel verwijderen?

@indiePeeters
Copy link
Contributor Author

Handmatig de bestaande records uit de Database verwijderen, met sql workbench of cli ofzo. De seeder zal ik vanavond even in elkaar zetten

@wouterbles
Copy link
Contributor

Gewoon een simpele SQL query draaien moet ook wel lukken denk ik. Dan maak ik wel eerst een backup ;)

@wouterbles
Copy link
Contributor

Handmatig de bestaande records uit de Database verwijderen, met sql workbench of cli ofzo. De seeder zal ik vanavond even in elkaar zetten

Dit nog gelukt?

@indiePeeters
Copy link
Contributor Author

@wouterbles zojuist de seeder in elkaar gezet en de env variabele naar de gehele url gezet

@wouterbles
Copy link
Contributor

Mooi! Heb je ook nog de API inlog gegevens van b2? Dan kan ik het even testen

@wouterbles
Copy link
Contributor

De photo album seeder werkte niet, die heb ik nog even gefixt.

Verder lukt het uploaden van albums bij mij niet, de progress bar blijft op 0% staan. Ik zie geen errors in de console, en ook in de database gebeurt helemaal niets.

@wouterbles
Copy link
Contributor

wouterbles commented Apr 2, 2019

Na veel te veel tijd heb ik eindelijk ontdekt waar het fout gaat sinds de overstap naar B2.

Backblaze is zoals ze het zelf zeggen erg goedkoop door hun speciale server architectuur. Hierdoor kan het voorkomen dat je bij een upload een 500/503 response terugkrijgt. Dit is te interpreteren als, probeer het nog een keer, vaak lukt het dan de tweede of derde keer wel. Meer informatie op deze blogpost: https://www.backblaze.com/blog/b2-503-500-server-error/

Nu blijkt dat het package wat wij gebriuken "laravel-backblaze-b2" depend op "flysystem-backblaze" en deze weer depend op "backblaze-b2". Die laatste is de PHP SDK. Om te kijken of een van deze packages 500/503 errors afvangt en retried heb ik de source van al deze packages doorgekeken. Dit is niet het geval, bij een 500/503 error geeft "backblaze-b2" een exception, iets wat dus volgens de blogpost hierboven niet zou moeten gebeuren.

Na even zoeken in de issues/PR's van de repo vond ik dit issue: gliterd/backblaze-b2#26. Naar mijn idee niet erg positief, een naar mijn idee legitiem issue wordt gesloten...

Om het bovenstaande probleem in de package op te lossen heb ik in mijn lokale versie van "backblaze-b2" het bestand \Http\Client.php aangepast met de volgende code:

        if ($response->getStatusCode() !== 200) {
            //ErrorHandler::handleErrorResponse($response);
            $response = parent::request($method, $uri, $options);
        }

Voor zover ik het heb kunnen testen lost dit onze problemen op!

Nu zijn er naar mijn idee twee dingen die moeten gebeuren:

  1. Een iets nettere try/catch/retry schrijven dan wat ik hierboven heb gemaakt. Hier ga ik mee aan de gang!
  2. Hierna maak ik een PR aan op de backblaze-b2 repo. Hopen dat de wijzigingen geaccepteert worden aangezien dat issue wel zomaar is gesloten.

UPDATE:
Met de testcode hierboven wordt elk bestand twee keer geupload naar backblaze,, waarom dit exact gebeurt weet ik niet. Maar dit is toch geen code die we in productie willen draaien.

UPDATE 2:
De issue die ik hierboven beschreef is inmiddels heropend 👍

@marijnvanderhorst
Copy link
Member

Heel netjes uitgezocht @wouterbles!
Mocht de PR niet op tijd geaccepteerd worden, lijkt het me het beste tijdelijk de fork op packagist.org te zetten.

@wouterbles
Copy link
Contributor

Dankje! Ik denk dat hij wel snel reageert, hij vroeg me om het openen van een PR.

@wouterbles
Copy link
Contributor

Nog een update:

Ipv laravel filesystem wordt nu direct de php sdk gebruikt. Dit haalt wat abstractie weg, maar maakt het wel een stuk makkelijker om van php sdk te wisselen. Anders zouden we de twee bovenliggende packages ook moeten forken en aanpassen. Bovendien heeft dit deze php sdk ook cahcing zodat we niet onze API caps halen.

Enige nadeel is dus dat hij de laatste twee commits nog niet heeft getagd in een release waardoor je een verouderde versie met bugs via composer binnen trekt. Ik heb daar een issue voor aangemaakt en hoop dat hij het fixt.

Wat denken jullie verder over de wijziging van filesystem naar sdk?

@wouterbles
Copy link
Contributor

Ik stel voor om met deze PR even te wachten en kijken of we iets met Azure kunnen. Dat zou namelijk veel makkelijker zijn.

@codecov-io
Copy link

codecov-io commented May 20, 2019

Codecov Report

Attention: Patch coverage is 9.52381% with 19 lines in your changes missing coverage. Please review.

Project coverage is 19.75%. Comparing base (b166424) to head (aa9cfa9).
Report is 559 commits behind head on master.

Files with missing lines Patch % Lines
app/repositories/PhotoRepository.php 0.00% 10 Missing ⚠️
app/Http/Controllers/PhotoController.php 0.00% 6 Missing ⚠️
app/MailList.php 0.00% 1 Missing ⚠️
app/Validators/ReCaptcha.php 0.00% 1 Missing ⚠️
app/repositories/PhotoAlbumRepository.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #73      +/-   ##
============================================
- Coverage     19.79%   19.75%   -0.04%     
  Complexity      620      620              
============================================
  Files            82       82              
  Lines          2117     2121       +4     
============================================
  Hits            419      419              
- Misses         1698     1702       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wouterbles
Copy link
Contributor

Ook al gaan we misschien over op azure, heb hier toch nog even naar gekeken en alles werkt tot nu toe goed, behalve twee dingen:

  • Afbeeldingen zijn nog steeds redelijk groot, soms wel 1.6MB. Op zich is dit geen heel groot probleem.
  • Er komen regelmatig backblaze errors omdat de env() functie soms 'null' returned. Even gezocht op deze error en het schijnt bad practice te zijn om direct de env() functie in je code te gebruiken wat waarschijnlijk zorgt voor deze error. Wij gebruiken eigenlijk overal env() direct in de code en zouden dit moeten verplaatsen naar config files om die vervolgens uit te lezen in de code.

@wouterbles
Copy link
Contributor

Alles werkt! Moet nu naar een meeting dus commit het straks.

@indiePeeters
Copy link
Contributor Author

Front end validatie fixen

IndieLeoPeeters added 4 commits October 11, 2019 13:09
# Conflicts:
#	composer.lock
#	package-lock.json
#	resources/views/front-end/agenda.blade.php
#	resources/views/front-end/zekeringen.blade.php
@wouterbles wouterbles merged commit 247133d into master Nov 24, 2019
@wouterbles wouterbles deleted the Photoalbums branch November 24, 2019 18:45
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.

5 participants