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

Move partial index into the typo3 database #138

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

Conversation

fseipel
Copy link
Member

@fseipel fseipel commented Jul 24, 2024

By moving the partial index from elasticsearch to typo3 it decouples the typo3 save process from elasticsearch. The current implementation results in long saving times in the typo3 backend if the Elasticsearch server was responding slow.

@fseipel fseipel marked this pull request as ready for review July 24, 2024 08:43
@fseipel fseipel changed the base branch from ES7V2 to master July 24, 2024 08:44
@fseipel fseipel changed the base branch from master to ES7V2 July 24, 2024 08:44
@mbrodala mbrodala marked this pull request as draft July 24, 2024 08:50
tags:
- '*'
pull_request:
branches:
- master
- '*'
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem unrelated and unnecessary, please revert them again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the change we have no CI on the pull request as it does not target the master branch.

Copy link
Member

Choose a reason for hiding this comment

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

In this case please add your temporary target branch instead. This ensures that there is no issue if this forgotten and merged as is.

Classes/Controller/BackendController.php Outdated Show resolved Hide resolved
Classes/Domain/Model/Update.php Outdated Show resolved Hide resolved
Classes/Domain/Model/Update.php Outdated Show resolved Hide resolved
Classes/Domain/Model/Update.php Outdated Show resolved Hide resolved
Classes/Query/UpdateQuery.php Show resolved Hide resolved
@@ -0,0 +1,55 @@
<?php

return [
Copy link
Member

Choose a reason for hiding this comment

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

We actually don't need any TCA for this table if we don't use the Extbase persistence.

ext_tables.sql Outdated Show resolved Hide resolved
ext_tables.sql Show resolved Hide resolved
ext_tables.sql Show resolved Hide resolved
}
}

public function pendingUpdates(string $type = null): array
Copy link
Member

@mbrodala mbrodala Jul 25, 2024

Choose a reason for hiding this comment

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

Personally I'd do this via IteratorAggregate::getIterator() instead. Then check if we can yield from the DBAL result. This would allow for consuming code to do this:

foreach ($this->updateQueue as $update) { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

As it needs to be filterable by type I think the current implementation is cleaner than the alternatives.

Using a Generator or passing the fetchAssociative directly (not a big fan) would be an option. But as it is only used in the preparation step to collect the to update uids and because of needed deduplication it should only a have minor impacts

@@ -0,0 +1,70 @@
<?php
namespace PAGEmachine\Searchable\Domain\Repository;
Copy link
Member

Choose a reason for hiding this comment

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

Domain may be OK but it's not a repository anymore, so it should be moved somewhere else.

Base automatically changed from ES7V2 to master July 30, 2024 14:27
tags:
- '*'
pull_request:
branches:
- master
- master
- ES7V2
Copy link
Member

@mbrodala mbrodala Jul 30, 2024

Choose a reason for hiding this comment

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

Can you rebase on master and drop these changes? #136 was merged. :-)

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.

2 participants