docker: add pulsar example#3789
Conversation
|
@codefromthecrypt Could you kindly review this when you have some time? Thank you! |
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
I think for this change, in order to be a proper dependency, should remove the docker-compose examples until there's server code to use them. Otherwise, I think it looks good to me as this should be enough to publish the image!
| container_name: pulsar | ||
| ports: # expose the pulsar port so apps can publish spans. | ||
| - "6650:6650" | ||
| - "8080:8080" |
There was a problem hiding this comment.
when ready for this change (docker-compose), note this is a common port, so maybe comment it out and say "uncomment to expose the UI port"
There was a problem hiding this comment.
Ok, I'll modify it when it's ready.
| # Usually, we read env set from pid 1 to get docker-healthcheck parameters. | ||
| # However, pulsar-server has to start as root even if permissions are dropped | ||
| # later. So, we expose it in the Dockerfile instead. | ||
| ENV HEALTHCHECK_PORT=8080 |
There was a problem hiding this comment.
shouldn't we check the service port 6650 (esp if we are using tcp)
There was a problem hiding this comment.
I modified it to HTTP on port 8080, which is enough for the healthcheck.
There was a problem hiding this comment.
ok mainly I was concerned about race condition where 8080 is ready, but service port not. If this becomes a problem we can revisit
|
looks like the healthcheck approach is failing on 404. you will need a valid http path also, once done please lower the log level to warn as it will be unreadable to have info due to the health checks (examples shouldn't scroll too long due to chatty or health check in docker, we usually set to WARN) |
|
Well, I have tested it locally and it should be OK now. |
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
Small charge then merge. Thanks!
|
It seems that the error is unrelated to the changes and may be due to a network issue, could you please run it again. |
3 participants
Since there is no collector-pulsar module at present, compiling docker will result in an error, so when this PR is merged, I will add
zipkin-collector-pulsarto the github actions in #3788.