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
Preregister Kafka Health #1023
Preregister Kafka Health #1023
Conversation
π¦ Changeset detectedLatest commit: 80be4a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 50 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
62e2394
to
c87ac7e
Compare
@@ -18,7 +18,7 @@ | |||
"images": "pnpm run:concurrent image", | |||
"preinstall": "npx check-node-version --package && npx only-allow pnpm && pnpm i -g pnpm@6.32.11", | |||
"lint": "pnpm lint:audit && pnpm lint:prettier '**/package.json' && pnpm lint:markdown && pnpm lint:eslint '**/*.ts'", | |||
"lint:audit": "pnpm-ci audit --strict -i GHSA-fwr7-v2mv-hh25", | |||
"lint:audit": "pnpm-ci audit --strict -i GHSA-fwr7-v2mv-hh25 GHSA-wm7h-9275-46v2", |
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.
We can undo this once expressjs/multer#1097 is merged.
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.
LGTM!
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.
Minor nit re delete
usage, but LGTM.
@@ -121,6 +126,7 @@ const createStreamConsumer = async ( | |||
await Promise.all(asArray(topics).map(topic => consumer.subscribe({ topic, fromBeginning }))) | |||
|
|||
registerLivelinessHandling(consumer, healthStatus) | |||
delete healthStatus.components[preConnectComponentId] |
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.
I don't love using delete
(I generally try to avoid), but the alternatives are somewhat cumbersome.
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.
It didn't seem worth doing another destructure and assignment when the delete would suffice
π Purpose
Register Kafka Health-Check components immediately, prior to successful connection.
π Resolves:
π¦ Impacts:
[long list TBD]
β PR Checklist