-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove default local domain for entries #1280
Merged
BentiGorlich
merged 8 commits into
main
from
fix/remove-remote-entries-from-local-domain
Dec 26, 2024
Merged
Remove default local domain for entries #1280
BentiGorlich
merged 8 commits into
main
from
fix/remove-remote-entries-from-local-domain
Dec 26, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When an entry is created it will be assigned a domain depending on its url. If it does not have one it previously fell back to the local domain. This commit removes this fallback as it can introduce long-running queries, when calculating the new number of entries in the domain Also added a command to remove all remote entries from this local domain (not a migration as we don't know which id the local domain has)
melroy89
reviewed
Dec 10, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the docs? Maybe after PR is merged: #1274
will do |
melroy89
previously approved these changes
Dec 25, 2024
melroy89
previously approved these changes
Dec 25, 2024
melroy89
reviewed
Dec 25, 2024
melroy89
approved these changes
Dec 25, 2024
BentiGorlich
added a commit
that referenced
this pull request
Dec 26, 2024
The domain changes from #1280 caused some tests to fail due to wrong assertions, fix that
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an entry is created it will be assigned a domain depending on its url. If it does not have one it previously fell back to the local domain. This commit removes this fallback as it can introduce long-running queries, when calculating the new number of entries in the domain
Also added a command to remove all remote entries from this local domain (not a migration as we don't know which id the local domain has).
What this looked like on Gehirneimer: there were >50,000 entries assigned to the domain
gehirneimer.de
. After removing the remote one it is down to 67. The problem with having this fallback is that you have to recount the number of entries in the domain. That could have been solved by just counting +1 instead of running a query (maybe in the future). The query for counting these entries took ~1.7 seconds, which is not terribly long, but it is avoidable and the domain should actually match the entry. No idea why the fallback was introduced