Added experimental background job queue #20985
Merged
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
Added config flags for the job queue to make it switchable config options are in services:jobs:queue and are 'enabled,reportStats,reportInterval,maxWorkers,logLevel(info/debug),pollMinInterval,pollMaxInterval,queueCapacity,fetchCount.
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
github-actions
bot
added
the
migration
cmraible
reviewed
Sep 12, 2024
cmraible
reviewed
Sep 12, 2024
cmraible
reviewed
Sep 12, 2024
cmraible
reviewed
Sep 12, 2024
cmraible
reviewed
Sep 12, 2024
cmraible
reviewed
Sep 12, 2024
This is slightly less efficient - potentially - because we do a select before inserting instead of ignoring the insert conflict. It's likely less efficient because I don't anticipate a ton of duplicates, although the analytics job can certainly result in that, which is where I'd expect the knex implementation to slightly win out. Regardless, testing is a fucking nightmare with knex directly as we have to spin up a db and use a schema for the table. Let's go down that path later if we need the performance improvements.
|
Need to add a couple integration tests then we ought to be ok. Not sure what the failure for Ghost-CLI is. |
|
It looks like there's an error that's preventing the persisted logs from being printed 🙃 Here's the culprint:
Looking at the lockfile changes, I'm not sure if they're intentional - there are a lot of new dependencies, which I think might stem from an older version of |
- moved to a more appropriate location (integration tests from e2e-server)
|
Ok, clearly there's some kind of race condition with the config mocks and initializing the service. I will have to look into that when I can reprioritize this. |
It appears testUtils.setup MUST be called as before(testUtils.setup()); and doesn't work as an anonymous function with any consistency.
This was failing locally due to the local test db (in /tmp/ghost-test.db) missing migration columns. Not sure why. It could be due to version mismatch at this point as the migration needs to be updated before merging this. Blowing away that file and then re-running init'd a table with the correct schema.
|
@vikaspotluri123 Hey, it seems like the |
|
Note to self: need to update migration to new version before merging. |
|
It took me a minute, but taking a look 👀 https://github.com/vikaspotluri123/Ghost/actions/runs/11393243539/job/31701108553?pr=14 |
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index eec2c09aeda..d68f13ce3a2 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -861,7 +861,8 @@ jobs:
- name: Update from v4
run: |
- ghost update -f -d $V4_DIR --archive $(pwd)/ghost/core/ghost.tgz
+ ghost update -f -d $V4_DIR --archive $(pwd)/ghost/core/ghost.tgz --no-restart
+ ghost run -d $V4_DIR -D
- name: Save Ghost CLI Debug Logs
if: failure()
@@ -879,7 +880,8 @@ jobs:
run: |
DIR=$(mktemp -d)
ghost install local -d $DIR
- ghost update -d $DIR --archive $(pwd)/ghost/core/ghost.tgz
+ ghost update -d $DIR --archive $(pwd)/ghost/core/ghost.tgz --no-restart
+ ghost run -d $DIR -D
- name: Print debug logs
if: failure()I hacked the job like that and found this |
|
Ah awesome, thank you. I suppose that's a good thing! I knew the migration needed updating but it's nice to know that it'll fail in CI. Should've thought of that. |
cmraible
approved these changes
Oct 31, 2024
tilak999
pushed a commit
to tilak999/ghost
that referenced
this pull request
Nov 20, 2024
ref https://linear.app/tryghost/issue/ENG-1556/ - added background job queue behind config flags - when enabled, is only used for the member email analytics updates in order to speed up the parent job, and take load off of the main process that is serving requests The intent here is to decouple certain code paths from the main process where it is unnecessary, or worse, where it's part of the request. Primary use cases are email analytics (particularly the member stats [open rate]) which are not particularly helpful in the period immediately following an email send, while the click traffic and delivered/opened events are. Related, the email link clicks themselves send off a cascade of events that are quite a burden on the main process currently and are somewhat tied to the request response when they needn't be. We'll be looking to tackle that after some initial testing with the email analytics job.
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.
ref https://linear.app/tryghost/issue/ENG-1556/