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

Plat 718 add kafka input output #205

Merged
merged 27 commits into from
Jul 17, 2023
Merged

Conversation

bradsawadye
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #205 (c4cc58d) into master (6684308) will increase coverage by 0.01%.
The diff coverage is 97.14%.

❗ Current head c4cc58d differs from pull request most recent head 55e9fa3. Consider uploading reports for the commit 55e9fa3 to get more accurate results

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   96.78%   96.79%   +0.01%     
==========================================
  Files          21       23       +2     
  Lines         964     1062      +98     
==========================================
+ Hits          933     1028      +95     
- Misses         31       34       +3     
Impacted Files Coverage Δ
src/config.js 100.00% <ø> (ø)
src/kafka.js 95.65% <95.65%> (ø)
src/kafkaUtils.js 95.91% <95.91%> (ø)
src/db/models/endpoints.js 100.00% <100.00%> (ø)
src/index.js 96.77% <100.00%> (+0.47%) ⬆️
src/middleware/externalRequests.js 98.22% <100.00%> (+<0.01%) ⬆️

The library kafkajs does not have the feature for unsubscribing a topic from a consumer. Work around for this is to disconnect the consumer and start a new instance of the consumer and resubscribe topics that should remain subscribed

PLAT-718
Support for kafka has been added to the mediator

PLAT-718
The app fails on node lts/gallium due to some libraries not being compatible with this version
This enables us to stub these function in tests
@bradsawadye bradsawadye marked this pull request as ready for review May 24, 2023 15:46
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
PLAT-718

const subscribeTopicToConsumer = async endpoint => {
if (endpoint.kafkaConsumerTopic) {
await kafkaConsumer.subscribe(endpoint.kafkaConsumerTopic, {

Choose a reason for hiding this comment

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

We need to either wrap this in a try/catch, or make sure that every place that calls it handles the error path (so make sure we call next(err) in the mongo models so we don't wait forever).

Currently if you have a kafka consumer endpoint saved but kafak is not running, if you try and start the project it will die ("uncaught KafkaJSConnectionClosedError: Closed connection").

Choose a reason for hiding this comment

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

This will allow for other endpoints to still run and does not create a hard dependency on kafka being up and running for the mapping mediator to function. The only issue now is the consumer will never subscribe if we start the mapping mediator if kafka was down at the same time. It may be a good idea to catch the ConnectionClosedError itself and then write polling logic to try and reconnect once kafka is up and running again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 55e9fa3

src/kafka.js Outdated
const subscribeTopicToConsumer = async endpoint => {
if (endpoint.kafkaConsumerTopic) {
await kafkaConsumer.subscribe(endpoint.kafkaConsumerTopic, {
url: `http://localhost:${port}/${endpoint.endpoint.pattern}`,

Choose a reason for hiding this comment

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

Suggested change
url: `http://localhost:${port}/${endpoint.endpoint.pattern}`,
url: `http://localhost:${port}${endpoint.endpoint.pattern}`,

Since endpoint.endpoint.pattern follows the pattern: "/pattern" we should remove the / otherwise we end up with //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 55e9fa3

Copy link

@arran-standish arran-standish left a comment

Choose a reason for hiding this comment

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

Looks good overall, just the one comment I left on the subscriber side of things

@bradsawadye bradsawadye merged commit c9cef59 into master Jul 17, 2023
4 checks passed
@bradsawadye bradsawadye deleted the PLAT-718-add-kafka-input-output branch July 17, 2023 12:07
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.

None yet

3 participants