Skip to content
This repository has been archived by the owner on Aug 20, 2021. It is now read-only.

Dynamic Addition for Exporters to Zeebe Cluster Containers #33

Merged
merged 4 commits into from Apr 8, 2020

Conversation

salaboy
Copy link
Collaborator

@salaboy salaboy commented Mar 9, 2020

This PR has an initial approach to be able to use initContainers to register new Exporters to the official Zeebe Docker Images.

This PR should promote not creating your own Docker Images for adding your custom exporters. It also made me think about removing the ElasticSearch one, as we can clearly document how to add one with initContainers hence having the base image more reusable and not bloated.

An example of how to add hazelcast and kafka can be found here:
https://github.com/zeebe-io/zeebe-helm-profiles/blob/master/examples/zeebe-cluster/exporters-values.yaml

Copy link

@saig0 saig0 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 for me. I added two minor comments.

@@ -22,43 +22,8 @@ data:
ZEEBE_CONTACT_POINTS="${ZEEBE_CONTACT_POINTS},${HOSTNAME::-1}$i.$(hostname -d):26502"
done
export ZEEBE_CONTACT_POINTS="${ZEEBE_CONTACT_POINTS}"
ls -al /exporters/
cp -a /exporters/*.jar /usr/local/zeebe/lib/
Copy link

Choose a reason for hiding this comment

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

I would prefer to copy the exporters into a separate folder (e.g. /usr/local/zeebe/exporters/) to avoid classpath issues.

zeebeCfg: |-
[[exporters]]
id = "elasticsearch"
className = "io.zeebe.exporter.ElasticsearchExporter"
Copy link

Choose a reason for hiding this comment

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

I think we should start to load the exporters from a defined jarPath to avoid classpath issues. For example, by adding jarPath = "exporters/zeebe-elasticsearch-exporter-0.22.1.jar" next to the exporters className.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@saig0 is that already there? I mean jarPath = can I use that already?

Copy link

@saig0 saig0 Mar 10, 2020

Choose a reason for hiding this comment

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

Yes, it is (since the beginning). We were just too lazy to use it 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@saig0 ok.. but I cannot make this change right now as the elasticsearch exporter is not in that new location right?
What I will do for now is to use your recommended path and in the examples, I will add the jarPath

Copy link

Choose a reason for hiding this comment

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

👍

@salaboy
Copy link
Collaborator Author

salaboy commented Mar 10, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saig0
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: salaboy

If they are not already assigned, you can assign the PR to them by writing /assign @salaboy in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@npepinpe
Copy link
Contributor

Generally looks fine for Zeebe pre 0.23.x - but from 0.23.x, injecting exporters becomes a piece of 🍰. Since we can now arbitrarily define exporters using environment variables, injecting exporters is now mostly just copying over the JAR to a pre-defined location and configuring your exporter. Whether we want to keep distributing these containers which contain the JAR + some default config is something to be discussed I guess.

@salaboy
Copy link
Collaborator Author

salaboy commented Mar 10, 2020

@npepinpe what would you expect to change for 0.23.x forward? I mean.. this is just showing how to do that exactly.. configure + copy the jars.. I consider this more like an example on how to do that. But if you are already working in big changes for how to plug new exporters please let me know so we can reflect that in the charts.

@Zelldon
Copy link
Collaborator

Zelldon commented Mar 26, 2020

What is the state of this? @npepinpe @salaboy

@salaboy
Copy link
Collaborator Author

salaboy commented Mar 26, 2020

@Zelldon I will try to rebase and re-test early next week.

@Zelldon Zelldon removed their request for review April 6, 2020 11:28
@salaboy salaboy force-pushed the init-containers-for-exporters branch from e5475d7 to f50a257 Compare April 8, 2020 09:25
@salaboy salaboy added size/S and removed size/M labels Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants