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

feat: add kafka importer as option #638

Merged
merged 3 commits into from
Dec 27, 2023

Conversation

gasymovrv
Copy link
Contributor

@gasymovrv gasymovrv commented Dec 17, 2023

Connected to the question:
#571

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2023

CLA assistant check
All committers have signed the CLA.

@nitram509
Copy link
Collaborator

@gasymovrv thank you for this PR - overall it looks quite good.
I have commented on a few things and would appreciate hearing your view on it.
Also, may I ask to extend the README, with some example(s) and description, please?

@gasymovrv gasymovrv changed the title feat: add kafka importer as option (first iteration without updating docs) feat: add kafka importer as option Dec 19, 2023
@gasymovrv gasymovrv requested a review from nitram509 December 19, 2023 14:27
Copy link
Collaborator

@nitram509 nitram509 left a comment

Choose a reason for hiding this comment

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

@gasymovrv
Ruslan, I did a local test and everything worked fine.
I made two comments. If you agree and would adjust these, then I will merge.
Again, great work 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, you though on updating this one.
Just being nitty gritty here, would you please adjust the arrows, following a consistent meaning (right now its mixed) - I propose 'technical dependency'.
Also, please adjust the text per each box being on the same semantical level, which makes reading easier.

See my proposed sketch, expressing what I have in mind...
Screenshot 2023-12-20 at 10 50 56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Сould you take a look to see if I understood you correctly (I didn't fully understand the DB dependency, you probably meant the arrow from ZSM to DB)?
how-it-works

Copy link
Collaborator

@nitram509 nitram509 Dec 20, 2023

Choose a reason for hiding this comment

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

good catch, you're right.
My proposal was wrong at this point 🙈

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I naively tried docker compose up it crashed on me, because multiple backends overlap/clash.
Also, firing up too many services will stress people, who don't have enough hardware resources.
While I like your idea of these prepared services defined, may I propose to introduce profiles.
E.g. there could be: hazelcast, kafka, postgres, mysql, and inmemory.

Which means, we could also add a small note in the CONTRIBUTION.md file,
saying something like: "For testing purpose, use docker profiles ... COMPOSE_PROFILES=inmemory,hazelcast docker compose up"

See also https://docs.docker.com/compose/profiles/

Copy link
Contributor Author

@gasymovrv gasymovrv Dec 21, 2023

Choose a reason for hiding this comment

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

Martin, it's a good idea, but I think there some troubles with multiple profiles. For example:
COMPOSE_PROFILES=in_memory,hazelcast docker compose up
will try to start all services that have "in_memory" OR "hazelcast" profile, because profiles works as "has any".
So each of these services matches:

  simple-monitor-in-memory:
    ...
    profiles: ["in_memory", "hazelcast"]

  simple-monitor-in-memory-kafka:
    ...
    profiles: ["in_memory", "kafka"]

  simple-monitor-postgres:
    ...
    profiles: ["postgres", "hazelcast"]

  simple-monitor-mysql:
    ...
    profiles: ["mysql", "hazelcast"]

The workaround is to use compound names for profiles and run docker-compose as follows: COMPOSE_PROFILES=hazelcast,hazelcast_postgres,postgres

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I see.
Thanks for pointing this out.

Copy link
Collaborator

@nitram509 nitram509 left a comment

Choose a reason for hiding this comment

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

Great job, Ruslan 🚀
Thank you very much for your contribution. 👍

@nitram509 nitram509 merged commit 64d4549 into camunda-community-hub:main Dec 27, 2023
3 checks passed
@gasymovrv
Copy link
Contributor Author

Thank you for the good comments! I hope this feature will be useful to people

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.

3 participants