Skip to content

Truncate tables and rename documents folder on reset - #5918

Merged
blizzz merged 3 commits into
mainfrom
fix/reset_sessions
Jun 17, 2024
Merged

Truncate tables and rename documents folder on reset#5918
blizzz merged 3 commits into
mainfrom
fix/reset_sessions

Conversation

@mejo-

Copy link
Copy Markdown
Member

📝 Summary

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

@juliusknorrjuliusknorr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 For the truncate part, would appreciate a review for my first commit then

@mejo-
mejo-force-pushed the fix/reset_sessions branch from b909a3a to d74e0edCompareJune 17, 2024 14:16
@mejo-mejo- changed the title Truncate tables and remove documents folder on resetTruncate tables and rename documents folder on resetJun 17, 2024
@mejo-
mejo-force-pushed the fix/reset_sessions branch from d74e0ed to e2f2244CompareJune 17, 2024 14:17
Comment threadlib/Command/ResetDocument.php Outdated
$fileIds = array_map(static function (Document $document) {
return $document->getId();
}, $this->documentService->getAll());
$fileIds = $this->documentService->getAll();

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@juliushaertl I don't understand this change. DocumentService->getAll() returns an array of Document objects, not the file IDs. How is this supposed to work?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Tested and indeed it broke with your current approach. The old approach didn't work either as array_map() cannot be run against a Generator. I pushed a fix:

$fileIds = [];
$documents = $this->documentService->getAll();
foreach ($documentsas$document) {
$fileIds[] = $document->getId();
}

@mejo-
mejo-force-pushed the fix/reset_sessions branch 3 times, most recently from 29cd087 to fe9a94fCompareJune 17, 2024 14:37
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@mejo-
mejo-force-pushed the fix/reset_sessions branch from fe9a94f to 7305078CompareJune 17, 2024 14:40
@mejo-

Copy link
Copy Markdown
MemberAuthor

would appreciate a review for my first commit then

Apart from the commented part, your changes look good to me @juliushaertl

Could you review my changes another time, given that I implemented the logic to rename the directory and clean up old directories after your last review?

@mejo-
mejo- requested review from blizzz and juliusknorrJune 17, 2024 14:43
mejo- added 2 commits June 17, 2024 17:02
This is way more performant than iterating over all existing sessions.
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
mejo-force-pushed the fix/reset_sessions branch from 7305078 to cd83e36CompareJune 17, 2024 15:02

public function clearAll(): void {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

truncate and delete are different things. Truncate might be faster than delete, but not all DB engines support it afaik. Not sure what is the current state in doctrine. OK for now.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I tested with 120.000 entries and it was super fast. So I guess it's ok for now 😬

@blizzzblizzzJun 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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


public function clearAll(): void {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as above

Comment threadlib/Db/StepMapper.php

public function clearAll(): void {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as above

@blizzz

Copy link
Copy Markdown
Member

/backport to stable29

@blizzz

Copy link
Copy Markdown
Member

/backport to stable28

@blizzz

Copy link
Copy Markdown
Member

/backport to stable27

@blizzz

Copy link
Copy Markdown
Member

Runner 9 was green before, Runner 1 is a known Flaky test → Merging.

@blizzz
blizzz merged commit 4e9ca5f into mainJun 17, 2024
@blizzz
blizzz deleted the fix/reset_sessions branch June 17, 2024 15:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Session cleanup

3 participants

@mejo-@blizzz@juliusknorr